[css-grid] Transferred min-size contribution of percentage size grid item with an intrinsic ratio

VERIFIED FIXED in Firefox 54

Status

()

Core
Layout
P1
major
VERIFIED FIXED
9 months ago
6 months ago

People

(Reporter: jensimmons, Assigned: mats)

Tracking

(Blocks: 2 bugs, {DevAdvocacy, testcase})

Trunk
mozilla55
DevAdvocacy, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox52 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54+ verified, firefox55+ verified)

Details

(Whiteboard: [DevRel:P1])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

9 months ago
I've built an example of a bug I found at: http://labs.jensimmons.com/2017/01-012-bug.html

Firefox doesn't seem to be handling `align-items: end` or `align-self: end` properly when there is more than one item placed in the same area. When those Grid containers are themselves items in a grandparent grid. The alignment seems to leak from the one Grid to the containing Grid, affecting it's alignment. Or something. 

See comparison with results in Chrome:
https://monosnap.com/file/bEj3Tm1PxuAI9z51lfK1UYMh1EGSPn.png
(Reporter)

Updated

9 months ago
Summary: Bug in Grid, when using alignment end → Bug in Grid, when using alignment end in nested Grid context
(Reporter)

Comment 1

9 months ago
Safari Technical Preview behaves as Chrome does.
Flags: needinfo?(mats)
(Reporter)

Comment 2

9 months ago
I'm having a hard time isolating where the problem is exactly. I'm not sure now that it's about nesting grids or align end. It seems to be a problem with the H2 height when I place items in the same cell. But... well, I'm really tired at the moment, so I'm having a hard time getting clear about what's happening. I hope you don't mind this messy bug report. 

I did add on some more attempts / variations to http://labs.jensimmons.com/2017/01-012-bug.html. I hope that helps. Maybe it muddles things up.
(Reporter)

Updated

9 months ago
Summary: Bug in Grid, when using alignment end in nested Grid context → Content height bug in CSS Grid, when attempting to overlay items

Comment 3

9 months ago
I think it is related to the images. The three boxes are exactly as tall as the natural height of their images (532, 720, 474).

If you add the CSS code `img { width: 300px }` the issues disappear.
(Assignee)

Comment 4

9 months ago
(Jen, FYI, if you label grid issues with [css-grid] it will turn up on my radar faster.)

I'm not entirely convinced this is a Gecko bug per the relevant specs just yet,
so I'm marking this "unconfirmed" for now.
Assignee: nobody → mats
Status: NEW → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(mats)
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Content height bug in CSS Grid, when attempting to overlay items → [css-grid] Transferred min-size contribution of percentage size grid item with an intrinsic ratio
(Reporter)

Comment 5

9 months ago
I think this is all about the height of images. 

Here's a better example: http://labs.jensimmons.com/2017/01-013-bug.html
(Without all the other factors involved.)

I don't know what the spec says — but here's my perspective as an author.
1) I've defined a grid, without saying anything about rows. So the rows should size themselves automatically to be the size of the content in the row.
2) I have images in the mix. And I've set img {width:100%} on them, a very common technique since responsive design took off. They are resizing themselves nicely, filling the width of the column space.  
3) The images also adjust height when they adjust width. And that's what I want. I've not specified a height on them. I don't want them cropped. They should maintain their aspect ratios. And they do.
4) I expect the rows to follow the height of the images. And adjust. But they don't. They are mysteriously much bigger than I expect. I look for a reason — Margin? Nope. Padding? Nope. What's going on? Can I control it? 
5) The rows are taking their heights from the actual size of the images, before they are resized by width:100%; As an author, this doesn't make sense to me. I don't know how or when this would be useful. It's also not what Chrome or Safari do. So it seems like a Firefox bug.
(Assignee)

Comment 6

9 months ago
Created attachment 8849801 [details]
Testcase #1

Here's a testcase that I think illustrates the core issue.
The image has width:100% which makes its inline 'min-content size' zero.
The (auto) column size is resolved as 300px - Chrome has the same value
so I think we can agree it's the correct column size.

Now, the interesting part - the (auto) row size.
The image has 'min-height:auto' so it has an 'automatic minimum size':
https://drafts.csswg.org/css-grid/#min-size-auto
(clamping does not apply, since all track max-sizing are 'auto'):

The image has 'height:auto' and an intrinsic ratio:
https://drafts.csswg.org/css-flexbox-1/#automatic-minimum-size
so the size should be 'the smaller of its content size and its transferred
size'.  The block-axis 'min-content size' is 720px.
Here's the definition of 'transferred size':
https://drafts.csswg.org/css-flexbox-1/#transferred-size
The 'computed cross size property' is 100%, but is that 'definite'?
Percentages are definite if the percentage base is definite, i.e. the grid
area's size in the relevant direction, which is calculated from the column
sizes in this case.  Is that considered 'auto' at this point or is it 300px?
https://drafts.csswg.org/css-grid/#algo-overview
Says "First, the track sizing algorithm is used to resolve the sizes of
the grid columns." so I guess they should be considered 'definite' for
the purpose of resolving percentages in the inline-direction (and thus
result in a transferred size).

Hmm, we actually do this for blocks:
http://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/layout/generic/nsGridContainerFrame.cpp#3760
But I guess we need something for the nsLayoutUtils::IntrinsicForAxis case.
The problem is our intrinsic sizing code does not actually take
a containing block size as input!
Instead we have this code, which uses the parent frame's specified size
as the percentage base which of course is completely bogus for grid items:
http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/layout/base/nsLayoutUtils.cpp#4668
(Assignee)

Comment 7

9 months ago
Yeah, I agree this is a bug now. :-)
Blocks: 616605
Status: UNCONFIRMED → ASSIGNED
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → wontfix
Ever confirmed: true
Keywords: testcase
(Reporter)

Updated

9 months ago
Whiteboard: [DevRel:P1]
(Reporter)

Comment 8

9 months ago
Person reporting this bug to me on Twitter this morning: https://twitter.com/icemagic/status/844523518326910976
Their demo: http://codepen.io/eystein/pen/MpVxvp
I created a test for the W3C suite with examples testing
what's described in https://github.com/w3c/csswg-drafts/issues/1117
The test is in this PR: https://github.com/w3c/csswg-test/pull/1247

It would be nice to verify that it's the expected behavior,
and in that case I hope it can be useful for this bug.

Comment 10

9 months ago
“The image has 'height:auto' and an intrinsic ratio:
https://drafts.csswg.org/css-flexbox-1/#automatic-minimum-size
so the size should be 'the smaller of its content size and its transferred size'.”

There's a difference between an automatic minimum size and an automatic size... so I'm not sure that analysis is quite right--height: auto is requesting an automatic size, which means using the formulas in CSS2.1 chapter 10.
(Reporter)

Comment 11

9 months ago
This is a really, really bad bug. I just ran into it again. 

Screenshot of problem: https://monosnap.com/file/PSAOcSxmmZp3TFz617MSTRnhM33E9B.png
See demo: http://labs.jensimmons.com/2017/01-016-bug.html

We are going to want to escalate this to the highest "fix now!" we've got for CSS.

Comment 12

9 months ago
It looks like Gecko is using the intrinsic standalone size of the image as its max-content size (in the grid algos) rather than using the output of the CSS2.1 chapter 10 rules. This wasn't correctly specified in Sizing before; it's since been updated to basically say "Look at CSS2.1 for how to calculate these sizes." It should follow the same rules as replaced blocks, basically.
(Reporter)

Updated

7 months ago
Severity: normal → critical
Priority: -- → P1
(Assignee)

Comment 13

7 months ago
FYI, critical is normally only used for crashes and security issues.
I agree this is a bad Grid layout bug though, sorry for taking so long
to get around to this.  A lot of higher prio bugs stole much of my time
lately. :(
Severity: critical → major
(Assignee)

Comment 14

7 months ago
Created attachment 8865125 [details] [diff] [review]
part 1 - [css-grid] Calculate (and cache) an item's percentage basis to use for resolving transferred percentages in intrinsic sizing.

This computes the percentage basis we want to use for grid items
in nsLayoutUtils::IntrinsicForAxis.  Not using it yet though -
that comes in the 2nd part.
Attachment #8865125 - Flags: review?(dholbert)
(Assignee)

Comment 15

7 months ago
Created attachment 8865127 [details] [diff] [review]
part 2 - [css-grid] Make nsLayoutUtils::IntrinsicForAxis take an optional percentage basis to use for resolving transferred percentages.

This makes IntrinsicForAxis use the aPercentageBasis if provided, which is
required for grid items.  Otherwise we'll use GetPercentBSize as before
to calculate one from our ancestors' content box as before, i.e. for all
non-Grid uses.

I'm also working on a follow-up part to refactor this code further.
The current calls GetPercentBSize are a bit inefficient since they'll
traverse the ancestor chain to calculate the percentage basis each time.
http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/layout/base/nsLayoutUtils.cpp#5242,5248,5260
It seems better to calculate a percentage basis once up front and then
let the new GetDefiniteSize function also handle the non-Grid case.
I.e. something like:
+        if (aPercentBasis.isNothing() &&
+            styleBSize->HasPercent() ||
+            styleMaxBSize->HasPercent() ||
+            styleMinBSize->HasPercent()) {
+          // calculate percentage basis from ancestors here
+        }

That part isn't urgent though, so it can be done in a follow-up.
Attachment #8865127 - Flags: review?(dholbert)
(Assignee)

Comment 16

7 months ago
Created attachment 8865128 [details] [diff] [review]
part 3 - [css-grid] Reftest updates for transferred percent intrinsic sizing fixes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f11e3cdfb58042f44dc66c9fadffeffe3c5a2a36
(Assignee)

Comment 17

7 months ago
Jen, here are Try builds available if you want to test this on your use cases:
https://archive.mozilla.org/pub/firefox/try-builds/mpalmgren@mozilla.com-f11e3cdfb58042f44dc66c9fadffeffe3c5a2a36/
(Assignee)

Comment 18

7 months ago
We should uplift this to v54 if possible.
status-firefox53: affected → wontfix
status-firefox-esr45: unaffected → wontfix
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
Track 54+ as css grid issue.
tracking-firefox54: ? → +
Jen, want to test this? 
Mats, I'm ok with just landing this on m-c if you like. 
We can also ask QA to have a look with the test in comment 11.
Flags: qe-verify+
Flags: needinfo?(jensimmons)
Flags: in-testsuite?
Andrei, can ask someone on your team to test with the try build? There is another test in comment 8, too.
tracking-firefox55: ? → +
Flags: needinfo?(andrei.vaida)
Comment on attachment 8865125 [details] [diff] [review]
part 1 - [css-grid] Calculate (and cache) an item's percentage basis to use for resolving transferred percentages in intrinsic sizing.

Review of attachment 8865125 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGridContainerFrame.cpp
@@ +1852,5 @@
> +   * If aUseIndefiniteSize is true then return NS_UNCONSTRAINEDSIZE in both axes.
> +   * Otherwise, assert that column sizes are known and calculate the size
> +   * for aGridItem.mArea.mCols and use NS_UNCONSTRAINEDSIZE in the other axis.
> +   */
> +  LogicalSize PercentageBasisFor(bool aUseIndefiniteSize,

It looks like every single caller passes "$some_logical_axis == eLogicalAxisInline" as the value of this boolean arg, and it's not superficially clear to me why that's correct.

To make that clearer, could you do one of the following:
 (1) Extend this function's aUseIndefiniteSize documentation here to explain why this might be the right thing for callers to pass. ("For example, if a caller is computing intrinsic inline size, then [...]")
...OR:
 (2) Just make this function take a LogicalAxis instead of a boolean arg, to coalesce all of the callers' common checks into one well-documented check inside of this function.

(I'm not sure which of those makes more sense. Also, see my comment on this function's impl, regarding what we return when this function is true.)

@@ +4856,5 @@
> +  const GridItemInfo& aGridItem) const
> +{
> +  auto wm = aGridItem.mFrame->GetWritingMode();
> +  if (aUseIndefiniteSize) {
> +    return LogicalSize(wm, NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE);

Observation: this function returns a LogicalSize which *always* gets wrapped in a Maybe<>, and then that Maybe<> ultimately (in the next patch) gets passed to GetDefiniteSize in nsLayoutUtils.cpp.  And that function bails if it encounters Nothing(), as well as if it encounters NS_UNCONSTRAINEDSIZE.

So: in light of that, I'm wondering whether there's a semantic difference between...
 (a) that Maybe<> being "Nothing()"
 (b) that Maybe<> containing LogicalSize(wm, NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE) -- the sentinel that we return here.

If there's no difference: perhaps this function shouldn't use NS_UNCONSTRAINEDSIZE for this special case, but should instead return a Maybe<LogicalSize> and should simply return Nothing() for this common special case?  That would simplify this early-return slightly, and it'd avoid the need to look up the writing-mode & construct a LogicalSize purely for this dummy sentinel value. And it'd make this sentinel value a bit more clearly sentinel-ish.
(In reply to Daniel Holbert [:dholbert] (AFK May 3-8, 13-21) from comment #22)
> I'm wondering whether there's a semantic difference
> between...
>  (a) that Maybe<> being "Nothing()"
>  (b) that Maybe<> containing LogicalSize(wm, NS_UNCONSTRAINEDSIZE,
> NS_UNCONSTRAINEDSIZE) -- the sentinel that we return here.

Hmm, after looking at part 2 more closely, I guess there is a semantic difference. So, disregard the second half of comment 22.

(I think this semantic difference needs to be more clearly explained, but that'll probably be a feature of Part 2.  More on this in my upcoming review feedback for that patch.)
Comment on attachment 8865125 [details] [diff] [review]
part 1 - [css-grid] Calculate (and cache) an item's percentage basis to use for resolving transferred percentages in intrinsic sizing.

Review of attachment 8865125 [details] [diff] [review]:
-----------------------------------------------------------------

[r=me on part 1 with first half of comment 22 addressed]
Attachment #8865125 - Flags: review?(dholbert) → review+
Comment hidden (obsolete)
Comment on attachment 8865127 [details] [diff] [review]
part 2 - [css-grid] Make nsLayoutUtils::IntrinsicForAxis take an optional percentage basis to use for resolving transferred percentages.

Review of attachment 8865127 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +4749,5 @@
> +      return false;
> +    }
> +    case eStyleUnit_Calc: {
> +      nsStyleCoord::Calc* calc = aStyle.GetCalcValue();
> +      if (MOZ_LIKELY(calc->mPercent != 0.0f)) {

Is this MOZ_LIKELY really justified?  It's not at all clear to me that it is.

Our eStyleUnit_Calc value here could just as easily be calc(1em + 5px), or calc(1px) [which looks silly but could be generated by some framework], or various other calc() formulations that don't involve a percent.

If we had a call to nsStyleCoord::HasPercent() further up (or conversely, ConvertsToLength()), this might make sense -- but in the absence of that, I don't think we can say that calc() with percent is "likely" enough to bother overriding the hardware's own branch predictor here [which might reasonably do a better job on its own].

(It's also not clear to me that this line is perf-critical enough to bother annotating -- and in particular, it's not perf-critical enough to bother doing the research/telemetry/whatever that'd be necessary convince ourselves that it'd be a sound annotation.)

@@ +4758,5 @@
> +        nscoord pb = aIsInlineAxis ? aPercentageBasis.value().ISize(wm)
> +                                   : aPercentageBasis.value().BSize(wm);
> +        if (pb == NS_UNCONSTRAINEDSIZE) {
> +          *aResult = std::max(0, nsLayoutUtils::AddPercents(calc->mLength,
> +                                                            calc->mPercent));

This case seems a little bit wrong.

This is a case where we have a calc() expression which includes a percent, and we have NS_UNCONSTRAINEDSIZE as our percent basis, and we're being asked whether that's convertable into something definite.  Per spec definitions of definite, I think the answer should be "no".  But right now we're happily "back-computing" the percent and returning true (indicating that we were able to resolve to a definite value, despite having a percent with no basis).

This seems wrong to me for two reasons:
 (1) a percent without a percent basis is indefinite by definition, IIUC. (So we should be returning false.)
 (2) higher up in the eStyleUnit_Percent cases, we return false if we have NS_UNCONSTRAINEDSIZE. So in that unconstrained-basis scenario, this code would behave differently for calc(50%) [which we'll resolve to 0 and return true] vs. plain old 50% [which will make us return false].  And that inconsistency seems wrong.

Perhaps this whole "if (pb == NS_UNCONSTRAINEDSIZE) ... else ..." chunk should be replaced with an early-return-false for unconstrained percent-basis, and a call to std::max(0, nsRuleNode::ComputeCoordPercentCalc()) otherwise?

Or perhaps this code is really correct, for some subtle reason? If so, that should be documented more clearly here.

@@ +4817,5 @@
>    }
>    return bSizeTakenByBoxSizing;
>  }
>  
> +// Get the amount of space taken out of aFrame's content area due to its

If this function ("GetDefiniteSizeTakenByBoxSizing") remains distinct from GetBSizeTakenByBoxSizing (per my comment below about sharing code), then the documentation needs to be made clearer about how to decide which function to call.

Right now the documentation is largely identical (aside from this one not being block-axis-specific).  But it looks like there's a behavioral difference -- they use different functions internally to resolve coord values (GetAbsoluteCoord vs. GetDefiniteSize).  So, the documentation should probably indicate this & hint at why it makes a difference.  Otherwise, someone who wants the block-axis size occupied by box-sizing would rightly be confused about whether they should call the older function, or should call this newer function with aIsInlineAxis=false.

@@ +4824,5 @@
> +// specific box-sizing values.
> +// aIsInlineAxis is true if we're computing for aFrame's inline axis.
> +// aIgnorePadding is true if padding should be ignored.
> +static nscoord
> +GetDefiniteSizeTakenByBoxSizing(StyleBoxSizing aBoxSizing,

This function looks copypasted-with-minor-tweaks from the existing GetBSizeTakenByBoxSizing() function.

Can we fix this up so that code is shared rather than copypasted? (Maybe by refactoring the existing function into something more generic, or into a templated helper?  Or maybe all GetBSizeTakenByBoxSizing callers should be adjusted to call this new function, and GetBSizeTakenByBoxSizing can already be replaced by this new version?)

@@ +4837,5 @@
> +      aIsInlineAxis == !aFrame->GetWritingMode().IsVertical();
> +    const nsStyleBorder* styleBorder = aFrame->StyleBorder();
> +    sizeTakenByBoxSizing =
> +      isHorizontalAxis ? styleBorder->GetComputedBorder().TopBottom()
> +                       : styleBorder->GetComputedBorder().LeftRight();

This logic looks backwards. If "isHorizontalAxis" is true, shouldn't we be getting LeftRight? (not TopBottom)

(Note that further down for padding, this function *does* get eSideLeft/eSideRight when |isHorizontalAxis| is true.  So even if the backwardness is correct for some reason, it's not consistently backwards right now.)

(In the function where this code is all copypasted from -- GetBSizeTakenByBoxSizing -- we do use TopBottom here, but the handling there is consistent between borders & padding, and it makes sense given the documentation there. In that function, aIsHorizontal refers to the inline axis & we're always dealing with *block-axis* measurements. But that's not the case in this new function.)

::: layout/base/nsLayoutUtils.h
@@ +1369,5 @@
>     * size for the given physical axis.  This considers the child's intrinsic
>     * width, its 'width', 'min-width', and 'max-width' properties (or 'height'
>     * variations if that's what matches aAxis) and its padding, border and margin
>     * in the corresponding dimension.
> +   * @param aPercentageBasis an optional percentage basis (in aFrame's WM)

Please adjust documentation to make clearer
 (1) that aPercentageBasis is allowed to have NS_UNCONSTRAINEDSIZE in either/both dimensions
 (2) what happens if aPercentageBasis is unspecified
 (3) what the semantic/behavioral difference is between these ^^ two ways of saying "no percent basis"
(Reporter)

Comment 27

7 months ago
I just tested with a build from Comment 16, testing these examples: 
http://labs.jensimmons.com/2017/01-012-bug.html
http://labs.jensimmons.com/2017/01-013-bug.html
http://labs.jensimmons.com/2017/01-016-bug.html
http://codepen.io/eystein/pen/MpVxvp

And all four look correct now. Yay! 

It would be terrific if this could land in FF54.
(Assignee)

Comment 28

7 months ago
(In reply to Daniel Holbert [:dholbert] (AFK May 3-8, 13-21) from comment #22)
>  (2) Just make this function take a LogicalAxis instead of a boolean arg, to
> coalesce all of the callers' common checks into one well-documented check
> inside of this function.

OK, we can do that for now.  We can easily change this again after it's clearer
what we should do for bug 1300366 (if anything) and what effects it might have
on percentage resolution.
(Assignee)

Comment 29

7 months ago
Created attachment 8866457 [details] [diff] [review]
part 2 - [css-grid] Make nsLayoutUtils::IntrinsicForAxis take an optional percentage basis to use for resolving transferred percentages.

(In reply to Daniel Holbert [:dholbert] (AFK May 3-8, 13-21) from comment #26)
> Is this MOZ_LIKELY really justified?

Fair enough.  Removed.

> > +          *aResult = std::max(0, nsLayoutUtils::AddPercents(calc->mLength,
> > +                                                            calc->mPercent));
> Or perhaps this code is really correct, for some subtle reason? If so, that
> should be documented more clearly here.

I believe it is the right thing to do for intrinsic sizing - but you're
right, I should probably motivate why more clearly.  I think I'll take
your advice and just return false here for now and file a follow-up bug
to investigate this more. (we have no tests that actually trigger this
line - I'll try to add some)

> This function looks copypasted-with-minor-tweaks from the existing
> GetBSizeTakenByBoxSizing() function.

The intention is to remove GetBSizeTakenByBoxSizing after I've made
everything take the GetDefiniteSize() path, so you can consider it
deprecated.  I'll remove it in the follow-up bug that will calculate
a percentage basis upfront also for non-Grid, as suggested in comment 15.

> > +      isHorizontalAxis ? styleBorder->GetComputedBorder().TopBottom()
> > +                       : styleBorder->GetComputedBorder().LeftRight();
> 
> This logic looks backwards. If "isHorizontalAxis" is true, shouldn't we be
> getting LeftRight? (not TopBottom)

Oops, I don't know how that snuck in there... fixed.

> ::: layout/base/nsLayoutUtils.h
> Please adjust documentation to make clearer

Fixed.
Attachment #8865127 - Attachment is obsolete: true
Attachment #8865127 - Flags: review?(dholbert)
Attachment #8866457 - Flags: review?(dholbert)
(Assignee)

Comment 30

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=245e3a9827553b976181f5e30f9766c8a5ce3a47
(Assignee)

Comment 31

7 months ago
Created attachment 8866539 [details] [diff] [review]
part 2 - [css-grid] Make nsLayoutUtils::IntrinsicForAxis take an optional percentage basis to use for resolving transferred percentages.

(Tweaked the documentation a bit.)
Attachment #8866457 - Attachment is obsolete: true
Attachment #8866457 - Flags: review?(dholbert)
Attachment #8866539 - Flags: review?(dholbert)
(In reply to Mats Palmgren (:mats) from comment #29)
> The intention is to remove GetBSizeTakenByBoxSizing after I've made
> everything take the GetDefiniteSize() path, so you can consider it
> deprecated.  I'll remove it in the follow-up bug that will calculate
> a percentage basis upfront also for non-Grid, as suggested in comment 15.

Could you add an XXX comment to GetBSizeTakenByBoxSizing, to indicate that it's doomed for removal? (Ideally with a bug number.)

Otherwise I worry we'll forget to remove it (or we won't forget, but in the meantime people might notice the two near-identical functions & be confused about which to use).
Comment on attachment 8866539 [details] [diff] [review]
part 2 - [css-grid] Make nsLayoutUtils::IntrinsicForAxis take an optional percentage basis to use for resolving transferred percentages.

Review of attachment 8866539 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, ideally with an XXX comment added per comment 32, and with the following:

::: layout/base/nsLayoutUtils.cpp
@@ +4765,5 @@
> +        if (pb == NS_UNCONSTRAINEDSIZE) {
> +          // XXXmats given that we're calculating an intrinsic size here,
> +          // maybe we should back-compute the calc-size using AddPercents?
> +          return false;
> +        } else {

drop else after return

::: layout/base/nsLayoutUtils.h
@@ +1372,5 @@
>     * width, its 'width', 'min-width', and 'max-width' properties (or 'height'
>     * variations if that's what matches aAxis) and its padding, border and margin
>     * in the corresponding dimension.
> +   * @param aPercentageBasis an optional percentage basis (in aFrame's WM),
> +   *   use NS_UNCONSTRAINEDSIZE if the basis is indefinite in either/both axes.

s/WM),/WM)./  (comma --> period)
s/use/Pass/   (clearer term, & capitalize to start new sentence).
Attachment #8866539 - Flags: review?(dholbert) → review+
(Assignee)

Updated

7 months ago
Blocks: 1363918
(Assignee)

Comment 34

7 months ago
Thanks for the review, I'll fix those nits.
I filed bug 1363918 on comment 15, which will remove GetBSizeTakenByBoxSizing.

Comment 35

7 months ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/523678fac831
part 1 - [css-grid] Calculate (and cache) an item's percentage basis to use for resolving transferred percentages in intrinsic sizing.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed40e2576953
part 2 - [css-grid] Make nsLayoutUtils::IntrinsicForAxis take an optional percentage basis to use for resolving transferred percentages.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/074b83d31e93
part 3 - [css-grid] Reftest updates for transferred percent intrinsic sizing fixes.
(Assignee)

Updated

7 months ago
Flags: in-testsuite? → in-testsuite+
I've managed to reproduce this issue on Fx 53 using links from comment 0 and comment 8. I can confirm this fix on the try builds, under the following OSes: Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x86 LTS.

All pages are displayed as expected now, on these examples:
- https://bug1349320.bmoattachments.org/attachment.cgi?id=8849801
- http://labs.jensimmons.com/2017/01-012-bug.html
- http://labs.jensimmons.com/2017/01-016-bug.html
- http://labs.jensimmons.com/2017/01-013-bug.html
- http://codepen.io/eystein/pen/MpVxvp
Flags: needinfo?(andrei.vaida)
Ciprian, did you test on 53 or on 54?
Flags: needinfo?(ciprian.georgiu)
I have verified that the issue is no longer reproducible with 55.0a1 try builds from comment 16.
Flags: needinfo?(ciprian.georgiu)
https://hg.mozilla.org/mozilla-central/rev/523678fac831
https://hg.mozilla.org/mozilla-central/rev/ed40e2576953
https://hg.mozilla.org/mozilla-central/rev/074b83d31e93
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please request Beta approval on this when you're comfortable doing so.
Flags: needinfo?(mats)
(Assignee)

Comment 41

7 months ago
Comment on attachment 8865125 [details] [diff] [review]
part 1 - [css-grid] Calculate (and cache) an item's percentage basis to use for resolving transferred percentages in intrinsic sizing.

Approval Request Comment
[Feature/Bug causing the regression]: CSS Grid
[User impact if declined]: broken transferred percentage sizes for image grid items
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:no
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:the changes should only affect Grid layout, because: A, it always provides aPercentageBasis to nsLayoutUtils::IntrinsicForAxis (in part 2) B, non-Grid calls do not, and C, all code paths internally should be the same when aPercentageBasis.isNothing().
[String changes made/needed]:none
Flags: needinfo?(mats)
Attachment #8865125 - Flags: approval-mozilla-beta?
(Assignee)

Comment 42

7 months ago
Hmm, do we actually need approval for bugs marked firefox54+ ?
Comment on attachment 8865125 [details] [diff] [review]
part 1 - [css-grid] Calculate (and cache) an item's percentage basis to use for resolving transferred percentages in intrinsic sizing.

Fix a css grid issue and was verified. Beta54+. Should be in 54 beta 8.
Attachment #8865125 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 44

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/ac0d18743eb5
https://hg.mozilla.org/releases/mozilla-beta/rev/47accb0093b9
https://hg.mozilla.org/releases/mozilla-beta/rev/73b1bdb6242f
status-firefox54: affected → fixed
Backed out for bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=99191170&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/241a875b457f
status-firefox54: fixed → affected
Flags: needinfo?(mats)
Debug-only it appears.
(Assignee)

Comment 47

7 months ago
Yeah, there was a dependency on bug 1360241 in an assertion, sorry about that.
I reverted that bit, here's the intradiff:
< +  MOZ_ASSERT(aFrame->GetParent()->Type() != LayoutFrameType::GridContainer ||
---
> +  MOZ_ASSERT(aFrame->GetParent()->GetType() != nsGkAtoms::gridContainerFrame ||
Flags: needinfo?(mats)
(Assignee)

Comment 48

7 months ago
uplift
https://hg.mozilla.org/releases/mozilla-beta/rev/7983fa68e19053f1b9a46d7b38f84916ba67716c
(Assignee)

Updated

7 months ago
status-firefox54: affected → fixed
This issue is verified fixed in latest Nightly 55.0a1 (2017-05-18) and 54 beta 9 (20170518105722) under the following OSes: Windows 10 x64, Mac OS X 10.10.5 and Ubuntu 16.04 x64 LTS.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
status-firefox55: fixed → verified
Flags: qe-verify+
(Reporter)

Updated

6 months ago
Duplicate of this bug: 1349481
You need to log in before you can comment on or make changes to this bug.