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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Simon Sapin (:SimonSapin)]

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.

Comment 1

[Simon Sapin (:SimonSapin)]

This only applies to elements that are not in a "display: none" subtree:
http://lists.w3.org/Archives/Public/www-style/2014Feb/0762.html

Comment 2

[Daniel Holbert [:dholbert]]

...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

Comment 3

[Simon Sapin (:SimonSapin)]

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>

Comment 4

[Tobias Schneider [:tobytailor]]

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?

Comment 5

[Tobias Schneider [:tobytailor]]

Attachment: Patch 1

Comment 6

[Tobias Schneider [:tobytailor]]

Attachment: Patch 1 (v2)

Forgot to include test in previous patch.

Comment 7

[Mats Palmgren (inactive)]

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.

Comment 8

[Mats Palmgren (inactive)]

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?

Comment 9

[Tobias Schneider [:tobytailor]]

Attachment: Patch 1 (v3): Resolve grid-template-{columns,rows} in px units.

Version 3 of the patch, addressing review comments.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 11

[Mats Palmgren (inactive)]

(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 12

[Mats Palmgren (inactive)]

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.

Comment 13

[Mats Palmgren (inactive)]

> remove one space before the second arg

Sorry, two spaces.  I need new glasses... :-)

Comment 14

[Tobias Schneider [:tobytailor]]

Attachment: Patch 1 (v4): Resolve grid-template-{columns,rows} in px units.

Addressing second batch of review comments. Added changes to get the right grid container within scrolled frames.

Comment 15

[Mats Palmgren (inactive)]

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.

Comment 16

[Tobias Schneider [:tobytailor]]

Attachment: Patch 1 (v5): Resolve grid-template-{columns,rows} in px units.

Nits addressed.

Comment 17

[Tobias Schneider [:tobytailor]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22dde34c710f

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c798c2576ad4

Comment 19

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e68851ffbe37a923ce7463b832dbfa23d01988bb
Backed out changeset c798c2576ad4 (bug 978212) for mochitest bustage

Comment 20

[Nigel Babu [:nigelb]]

Backed out for bustages in M(5) https://treeherder.mozilla.org/logviewer.html#?job_id=18374784&repo=mozilla-inbound

Comment 21

[Mats Palmgren (inactive)]

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...

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5138ad90e360

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5138ad90e360