Closed Bug 978212 Opened 6 years ago Closed 4 years ago

Resolved value of grid-template-{columns,rows} in px units

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: SimonSapin, Assigned: tschneider)

References

(Blocks 2 open bugs, )

Details

Attachments

(1 file, 4 obsolete files)

The resolved value (used for getComputedStyle()) of the grid-template-columns and grid-template-rows properties should have track sizes given in pixels, regardless of the specified track sizing functions. For flexible and intrinsi sizing functions, this requires results from layout (reflow).

http://dev.w3.org/csswg/css-grid/#resolved-track-list

Bug 976787 is adding style system support for these properties. Because there is no reflow code yet, it (incorrectly) leaves non-length sizing functions as specified in the resolved value.
This only applies to elements that are not in a "display: none" subtree:
http://lists.w3.org/Archives/Public/www-style/2014Feb/0762.html
...which I believe means that in nsComputedDOMStyle, we can skip resolving to px units if "mInnerFrame" is null. This is what we do in e.g. nsComputedDOMStyle::DoGetHeight() -- it looks like this:

> 3840   bool calcHeight = false;
> 3842   if (mInnerFrame) {
> 3843     calcHeight = true;
>          [...snip...]
> 3853   }
> 3854 
> 3855   if (calcHeight) {
>          [...code to resolve to PX values ...]
> 3860   } else {
>          [...fallback code for display:none case...]
> 3874   }

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp?rev=22a2eb6d2145#3835
Depends on: 976787
According to http://lists.w3.org/Archives/Public/www-style/2014Mar/0132.html , this does *NOT* apply to the grid-auto-columns and grid-auto-rows properties, which share the same serialization code for <track-size>
Blocks: 1217086
Assignee: nobody → schneider
After getting somehow familiar with the existing code, my first attempt would be moving mCols and mRows from GridReflowState to nsGridContainerFrame and using the calculated track sizes in nsComputedDOMStyle::GetGridTemplateColumnsRows. Does that sound like a good point to start from?
Flags: needinfo?(simon.sapin)
Flags: needinfo?(mats)
Attached patch Patch 1 (obsolete) — Splinter Review
Flags: needinfo?(simon.sapin)
Flags: needinfo?(mats)
Attachment #8691680 - Flags: review?(mats)
Attached patch Patch 1 (v2) (obsolete) — Splinter Review
Forgot to include test in previous patch.
Attachment #8691680 - Attachment is obsolete: true
Attachment #8691680 - Flags: review?(mats)
Attachment #8691683 - Flags: review?(mats)
Attachment #8691683 - Attachment description: Part 1 (v2) → Patch 1 (v2)
Comment on attachment 8691683 [details] [diff] [review]
Patch 1 (v2)

Looks good!  Thanks for picking this up.

>layout/generic/nsGridContainerFrame.cpp
>+  nsTArray<nscoord> colTrackSizes;
>+  colTrackSizes.SetCapacity(gridReflowState.mCols.mSizes.Length());

I think you can give the initial capacity to the ctor.
(for better performance)

>+  Properties().Set(GridColTrackSizes(),
>+                   new nsTArray<nscoord>(mozilla::Move(colTrackSizes)));

This is good for now but I'm slightly worried about the performance
impact of doing this on every reflow.  I think we want to move this
into a separate function eventually and only set this frame property
when it's needed.  But this is good enough for now I think.
Please file a follow-up bug on that though?

What you can do in this bug is to add a couple of accessor methods
(ComputedTemplateColumns/Rows?) on nsGridContainerFrame returning
a pointer to the array, and use those in the nsComputedDOMStyle code
rather than accessing the frame properties directly.  Then we can
modify the methods later in the follow-up bug.  (It encapsulates
implementation details too.)

>layout/generic/nsGridContainerFrame.h
>+    if (aTrackSizes) {
>+      nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;
>+      val->SetAppUnits(aTrackSizes->ElementAt(i));
>+      valueList->AppendCSSValue(val);
>+    } else {
>+      valueList->AppendCSSValue(GetGridTrackSize(aTrackList.mMinTrackSizingFunctions[i],
>+                                                 aTrackList.mMaxTrackSizingFunctions[i]));
>+    }

You couldn't know this but this wont work for 'auto-fill/fit' tracks,
which I'm adding soon.  So please split this up into two loops, one for
aTrackSizes and the other for the original code (as is) when it's null.
Then loop over all entries in aTrackSizes which may have less or more
entries than |numSizes|.

>layout/style/test/test_grid_computed_values.html

Please add a test here that causes a reflow on the grid container
(setting its 'width' or something) and then check the values again.
Attachment #8691683 - Flags: review?(mats) → feedback+
It would help to have a little more context for your patches.
Can you add "unified = 8" to the [diff] section in your .hgrc please?
Version 3 of the patch, addressing review comments.
Attachment #8691683 - Attachment is obsolete: true
Attachment #8693913 - Flags: review?(mats)
Generally we use properties for things that only exist rarely, to avoid taking the space of a member variable.  But it looks like these are set unconditionally, in which case they should probably be member variables rather than properties.

That said -- how much space do these take up?  This seems like a lot of overhead to get a value serialized in a particular way -- maybe it makes more sense to try to get the spec changed?
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #10)
> Generally we use properties for things that only exist rarely, to avoid
> taking the space of a member variable.  But it looks like these are set
> unconditionally, in which case they should probably be member variables
> rather than properties.

Yeah, I already pointed that out above, but I think this is fine for now.
We can optimize it in a follow-up bug.  A expect we'll want to share grid
specific data between fragments too, once I get to implementing that.
It might be worth waiting for that to see if we can share some code.

> This seems like a lot of overhead to get a value serialized in a particular way

It may have been designed this way for compatibility with IE's old
proprietary grid layout?

> maybe it makes more sense to try to get the spec changed?

I'm quite certain we need to provide some way for web developers to
get at the (used values for) track dimensions, and how many there are,
which is dynamic with auto-fill/fit.  (I expect they will request much
more than this actually; if we use the DevTools wishlist as guidance:
https://old.etherpad-mozilla.org/Devtools-CSS-Grids)
Comment on attachment 8693913 [details] [diff] [review]
Patch 1 (v3): Resolve grid-template-{columns,rows} in px units.

r=mats with the following addressed as you see fit:

>layout/generic/nsGridContainerFrame.h
>+  static nsTArray<nscoord>* GetComputedTemplateRows(nsIFrame* aFrame)

I think these should be ordinary methods, unless there's a reason
why they must be 'static'.  Please also add 'const' to the return
type.

>layout/style/nsComputedDOMStyle.cpp
>+      nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;

Make the * go with the type: "nsROCSSPrimitiveValue* "

>+      valueList->AppendCSSValue(GetGridTrackSize(aTrackList.mMinTrackSizingFunctions[i],
>+                                                   aTrackList.mMaxTrackSizingFunctions[i]));

remove one space before the second arg

>+  if (mInnerFrame && mInnerFrame->GetType() == nsGkAtoms::gridContainerFrame) {
>+    nsIFrame* gridContainer = static_cast<nsGridContainerFrame*>(mInnerFrame);

This pattern is better written as:
  auto gridContainer = static_cast<...

>+    trackSizes = nsGridContainerFrame::GetComputedTemplateColumns(gridContainer);

After removing 'static' and using 'auto' as suggested above, this becomes:
  trackSizes = gridContainer->GetComputedTemplateColumns();

Please also add a test for a "overflow:scroll" grid element, just to make
sure we have the right "mInnerFrame" here.
Attachment #8693913 - Flags: review?(mats) → review+
> remove one space before the second arg

Sorry, two spaces.  I need new glasses... :-)
Addressing second batch of review comments. Added changes to get the right grid container within scrolled frames.
Attachment #8693913 - Attachment is obsolete: true
Attachment #8695128 - Flags: review?(mats)
Comment on attachment 8695128 [details] [diff] [review]
Patch 1 (v4): Resolve grid-template-{columns,rows} in px units.

r=mats with the nits below addressed:

>layout/style/nsComputedDOMStyle.cpp
>+    if (gridContainerCandidate->GetType() == nsGkAtoms::scrollFrame) {
>+      nsIScrollableFrame* scrollFrame = do_QueryFrame(gridContainerCandidate);
>+      gridContainerCandidate = scrollFrame->GetScrolledFrame();
>+    }

That works, but I'd prefer if you replace that whole if-statement with:

  gridContainerCandidate = gridContainerCandidate->GetContentInsertionFrame();

Then it will work with arbitrary wrapping frame(s) around the GridContainerFrame.
(e.g. <fieldset>, once we get to that: bug 1230207)

>+    if (gridContainerCandidate && gridContainerCandidate->GetType() == nsGkAtoms::gridContainerFrame) {

This line is a bit long, please move the second term to a new line.
Attachment #8695128 - Flags: review?(mats) → review+
Nits addressed.
Attachment #8695128 - Attachment is obsolete: true
Keywords: checkin-needed
TEST-UNEXPECTED-FAIL | layout/style/test/test_grid_computed_values.html | test computed grid-template-{columns,rows} values, overflow: scroll - test computed grid-template-{columns,rows} values, overflow: scroll: assert_equals: expected "[a] 50px [b] 320px [b c d e] 40px [e] 40px 0px 0px 0px 0px 50px" but got "[a] 50px [b] 307px [b c d e] 40px [e] 40px 0px 0px 0px 0px 50px"

The difference between the expected value 320px and the actual value 307px is the size
of the scrollbar.  This is a bug in the test not the code.  I've fixed the test so
that it accounts for a potential scrollbar and will reland the patch with that...
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/5138ad90e360
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1234311
Depends on: 1281446
You need to log in before you can comment on or make changes to this bug.