Open Bug 1349677 Opened 9 years ago Updated 3 years ago

Provide the authored styles for a grid cell

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

People

(Reporter: gl, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

Bug 1349677 Part 4: Cleanup grid tests to replace tabs with spaces and remove extra whitespace.
59 bytes, text/x-review-board-request
Details
Bug 1349677 Part 1: Carry grid track authored size into ComputedGridTrackInfo.
59 bytes, text/x-review-board-request
Details
Bug 1349677 Part 2: Add an empty authoredSize property to GridTrack.
59 bytes, text/x-review-board-request
Details
Bug 1349677 Part 3: Supply the GridTrack authoredSize property with data from the ComputedGridTrackInfo.
59 bytes, text/x-review-board-request
Details
Bug 1349677 Part 5: Updated grid tests to check authored sizes.
59 bytes, text/x-review-board-request
Details
GetGridTrackSize-hack
3.84 KB, patch
Details | Diff | Splinter Review
In DevTools, we would like to provide the authored and computed dimensions of a grid cell when inspecting / highlighting a grid cell and visually seeing their row / col number and their dimensions. See https://projects.invisionapp.com/share/3X87NEBYH#/screens/179720294_Grid Currently, we have getFragmentData() that provides a lot of useful information. I would imagine adding a "cells" property that is a 2D array that would provide us the authoredWidth, authoredHeight for the cell at a given row and column number. Alternatively, I think this makes sense in the GridTrack as well. You can see how we utilize the grid fragment data to render a grid cell in the canvas that it use to provide a grid overlay on the page here: http://searchfox.org/mozilla-central/source/devtools/server/actors/highlighters/css-grid.js#1005
Blocks: dt-grid
Blocks: 1349688
Status: NEW → ASSIGNED
Attachment #8850772 - Flags: review?(mats)
Attachment #8850773 - Flags: review?(mats)
Attachment #8850774 - Flags: review?(cam)
Attachment #8850775 - Flags: review?(mats)
Attachment #8850776 - Flags: review?(mats)
Attachment #8850777 - Flags: review?(mats)
Comment on attachment 8850772 [details] Bug 1349677 Part 4: Cleanup grid tests to replace tabs with spaces and remove extra whitespace. https://reviewboard.mozilla.org/r/123288/#review126022 ::: layout/style/nsStyleStruct.h:1603 (Diff revision 2) > + nsTArray<nsString> mTrackAuthoredValues; > + nsString mAutoAuthoredValue; I'm a little worried about the added memory cost of storing strings on the style structs for this. It seems like a high price to pay for everyone just for the potential use by devtools. We already code for serializing these values in: http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/layout/style/nsComputedDOMStyle.cpp#2985-3020 Couldn't we use something like that instead? (although you probably want to write it more like the code in the if-branch so you get the implicit and repeated tracks correct; more on that in the next comment...)
Attachment #8850772 - Flags: review?(mats) → review-
Comment on attachment 8850773 [details] Bug 1349677 Part 1: Carry grid track authored size into ComputedGridTrackInfo. https://reviewboard.mozilla.org/r/123290/#review126024 ::: layout/generic/nsGridContainerFrame.cpp:6272 (Diff revision 2) > + int32_t authoredIndex = col - > + gridReflowInput.mColFunctions.mExplicitGridOffset; > + if (authoredIndex >= 0 && > + authoredIndex < (int32_t)colTemplate.mTrackAuthoredValues.Length()) { > + colTrackAuthoredSizes.AppendElement( > + colTemplate.mTrackAuthoredValues[authoredIndex]); > + } else { > + // An implicit track, so authored size comes from grid-auto-columns. > + colTrackAuthoredSizes.AppendElement(colTemplate.mAutoAuthoredValue); > + } > bool isRepeat = ((col >= gridReflowInput.mColFunctions.mRepeatAutoStart) && This doesn't seem right. For example: grid-template-rows: repeat(auto-fill, 20px); height: 200px; here we will only have one value in mTrackAuthoredValues (IIUC) so the other generated tracks will be reported as implicit tracks. (I think you need something like the code I linked to earlier to get this right.) BTW, nsGridContainerFrame::TrackSizingFunctions also have code to map a track index to the relevant nsStyleCoord: http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/layout/generic/nsGridContainerFrame.cpp#1252-1281 That's perhaps easier to use from the code in Reflow that sets up the value for devtools, assuming you can figure out a way to call nsComputedDOMStyle::GetGridTrackSize from there.
Attachment #8850773 - Flags: review?(mats) → review-
Attachment #8850774 - Flags: review?(cam) → review?(mats)
Probably best for Mats to review part 3 along with the rest.
(In reply to Mats Palmgren (:mats) from comment #13) > We already code for serializing these values in: > http://searchfox.org/mozilla-central/rev/ > 7419b368156a6efa24777b21b0e5706be89a9c2f/layout/style/nsComputedDOMStyle. > cpp#2985-3020 > Couldn't we use something like that instead? > (although you probably want to write it more like the code in the if-branch > so you get the implicit and repeated tracks correct; more on that in > the next comment...) Sure, much of what devtools wants is available by calling element.getComputedStyle().getGridTemplateColumns()/Rows(), but that wouldn't correctly handle authored sizes with functions, calc, percentages or "fr". I assume getting that additional level of correctness for authored styles is the intent of this bug. Gabe?
Flags: needinfo?(gl)
Yes, the intention is to be able show both the authored and computed style for a given grid cell.
Flags: needinfo?(gl)
Attached patch GetGridTrackSize-hack — — Splinter Review
There seems to be some misunderstanding here, I'm not suggesting we should report the "px" sizes that element.getComputedStyle().getGridTemplateColumns()/Rows() does. Note that those two are special as they report the resolved values: https://drafts.csswg.org/css-grid/#resolved-track-list We have the real computed values in the style system, which we can serialize. That would be very close to what the author specified, although they would be canonicalized, e.g. 'minmax(auto, auto)' will be serialized as 'auto' etc. I don't think this minor difference to what the author specified is important. Authors generally type the minimum forms anyway, and if they don't, I think they will appreciate to learn they don't have to type out 'minmax(auto, auto)'. :-) For example, for this testcase: data:text/html,<div style="display:grid; grid-template-rows: min-content 1% 1fr calc(1px + 1%) auto minmax(min-content, 1fr) minmax(auto, auto) repeat(2, 1fr)"> this patch prints: min-content 1% 1fr calc(1px + 1%) auto minmax(min-content, 1fr) auto 1fr 1fr
(In reply to Mats Palmgren (:mats) from comment #18) > We have the real computed values in the style system, which we can serialize. Thank you for demonstrating this, Mats. I'll put together a patch that delivers these values via the proposed authoredSize property on the GridTrack object.
Attachment #8850774 - Flags: review?(mats)
Attachment #8850775 - Flags: review?(mats)
Attachment #8850776 - Flags: review?(mats)
Attachment #8850777 - Flags: review?(mats)
No longer blocks: 1349688
(In reply to Mats Palmgren (:mats) from comment #18) > We have the real computed values in the style system, which we can serialize. As I start to revise this using nsComputedDOMStyle as you propose, I see that we don't have the computed values for repeat(auto-fit) tracks when all the tracks are removed. We'll have to capture the authored size at some point before the style system drops all the empty tracks.
I'm pretty sure the style system always have all values. We have all the state we need about the tracks here: http://searchfox.org/mozilla-central/rev/6e1c138a06a80f2bb3167d9dac050cc0fd4fb39e/layout/generic/nsGridContainerFrame.cpp#1064-1073 and the style for the repeat tracks can be found here as usual: http://searchfox.org/mozilla-central/rev/6e1c138a06a80f2bb3167d9dac050cc0fd4fb39e/layout/style/nsStyleStruct.h#1608-1609,1612 You need something else to iterate than my WIP showed though, because that only iterates tracks that actually exist. I think nsGridContainerFrame::TrackSizingFunctions has all the info needed so probably add a method there?
(In reply to Mats Palmgren (:mats) from comment #21) > and the style for the repeat tracks can be found here as usual: > http://searchfox.org/mozilla-central/rev/ > 6e1c138a06a80f2bb3167d9dac050cc0fd4fb39e/layout/style/nsStyleStruct.h#1608- > 1609,1612 If I'm reading it correctly, theses values won't be present for repeat auto-fit tracks that are all removed -- if all of them are removed like with grid-template-columns: 1fr repeat(auto-fit, 1e+10px). We'll have to carry those min and max nsStyleCoords explicitly in such a case.
(In reply to Brad Werth [:bradwerth] from comment #22) > If I'm reading it correctly, theses values won't be present for repeat > auto-fit tracks that are all removed -- if all of them are removed like with > grid-template-columns: 1fr repeat(auto-fit, 1e+10px). We'll have to carry > those min and max nsStyleCoords explicitly in such a case. (replying to myself) ...but then there's no track to get the authored size of. It looks like we don't need to worry about the "all the auto-fit tracks are removed" case. That simplifies things.
(In reply to Brad Werth [:bradwerth] from comment #22) > If I'm reading it correctly, theses values won't be present for repeat > auto-fit tracks that are all removed -- if all of them are removed like with > grid-template-columns: 1fr repeat(auto-fit, 1e+10px). We'll have to carry > those min and max nsStyleCoords explicitly in such a case. All repeat(auto-fill/fit) tracks have the same style: mMin/MaxTrackSizingFunctions(mRepeatAutoIndex) and that value is there whether any/all tracks were removed or not. You just have to determine if the track is in the repeat-range and return that value if so. I think TrackSizingFunctions has all the info needed for that. I don't see why the "all tracks were removed" case is any different from the "some tracks were removed" case.
(In reply to Mats Palmgren (:mats) from comment #24) > I don't see why the "all tracks were removed" case is any different from > the "some tracks were removed" case. I've confirmed this is a problem with the current code path, as the nsStyleGridTemplate gets transformed into the ComputedGridTrackInfo. If all the repeat auto-fit tracks are removed, then the code at http://searchfox.org/mozilla-central/source/layout/generic/nsGridContainerFrame.cpp#714 sets mRepeatAutoStart to 0, though the mSizes array remains 0 length. In that situation, there's no way to retrieve the size of the removed repeat tracks from the ComputedGridTrackInfo. The minimal intervention may be to specially carry the repeat track size in ComputedGridTrackInfo to handle this case. The patch I'll push shortly will use this approach.
Attachment #8850776 - Attachment is obsolete: true
Priority: -- → P3
I'm not actively working on this anymore. Taking myself off the bug.
Assignee: bwerth → nobody
Status: ASSIGNED → NEW
See Also: → 1627347
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: