Closed Bug 1298008 Opened 8 years ago Closed 6 years ago

node.getBoxQuads doesn't return the right dimensions for block elements with auto horizontal margins

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: pbro, Assigned: bradwerth)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [designer-tools][wptsync upstream])

Attachments

(7 files)

missing-auto-margin.html
291 bytes, text/html
Details
Bug 1298008 Part 1: Update GeometryUtils::GetBoxRectForFrame to use GetMarginRectRelativeToSelf for margin boxes.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1298008 Part 2: Make nsFlexContainer update UsedMargin property after final reflow.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1298008 Part 3: Update ReflowInput::CalculateBlockSideMargins to store computed values in the UsedMargin property.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1298008 Part 4: Add a web-platform test of getBoxQuads on block and flex boxes with auto margins.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1298008 Part 5: Change a web-platform test that checks getComputedStyle to PASS.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1298008 Part 6: Update a devtools test that checks the computed values of auto margins in the box model.
59 bytes, text/x-review-board-request
gl
: review+
Details 
STR:

- open the attached test case
- notice that the yellow box is horizontally centered thanks to margin: 0 auto;
- open the devtools console
- evaluate the following 2 expressions in the console:

$("div").getBoxQuads({box: "border"})[0].bounds
$("div").getBoxQuads({box: "margin"})[0].bounds

Actual: both bounds have the same width.
Expected: I would expect the margin bound to have a different width, since there is indeed a margin left and right of the element.

Note, getBoxQuads was implemented in bug 917755.
Also, note that this impacts the devtools inspector. You can see it by opening the inspector and moving your mouse over the <div></div> line in the inspector. You should see the highlighter appear in the page, and you should see that the margin is not being shown.
Priority: -- → P3
Hey Brad, I thought maybe you could look at this one and check what the problem is.
It's making our highlighter incorrect in auto margin situations.
Flags: needinfo?(bwerth)
This isn't as simple as I had hoped. The issue is that we compute and store a "UsedMargin" property for each frame, but that computed value is unit-less, and is given a 0 value for auto margins in nsLayoutUtils::ComputeCBDependentValue(). Weird! Presumably that's because auto margins are actually computed within reflow and this property is known to be junk for auto margins. A comment in SizeComputationInput::InitOffsets() seems to imply that.

So, the solutions I can imagine are:
1) Whenever we actually do calculate the used values for auto margins in reflow, update the UsedMargin property. This has a very very small but non-zero performance cost for all reflows.
2) Make UsedMargin an nsStyleCoord and when we retrieve an auto value, we do our own auto size calculation in nsIFrame::GetMarginRectRelativeToSelf(). Calculating it here would also be a small performance cost, and perhaps hard to guarantee is exactly the same as what is used by layout (or it could be guaranteed to be the same), but the cost is only paid on-demand. This method is only called from various chrome-only *Utils classes, so whatever cost is incurred is only paid by the tools.

I'm sensitive to dangers of regressing performance, but I would prefer to pursue option 1, because it would be guaranteed to be the same as the layout value. Daniel, what do you think?
Flags: needinfo?(bwerth) → needinfo?(dholbert)
Assignee: nobody → bwerth
(In reply to Brad Werth [:bradwerth] from comment #2)
> So, the solutions I can imagine are:
> 1) Whenever we actually do calculate the used values for auto margins in
> reflow, update the UsedMargin property. This has a very very small but
> non-zero performance cost for all reflows.

This is what we should do, I think.  I think technically this is what we're supposed to already be doing - see https://dxr.mozilla.org/mozilla-central/rev/c4ebc8c28a33b785dfbfa533810517cc707d1ad0/layout/generic/nsFlexContainerFrame.cpp#4867-4869

> 2) Make UsedMargin an nsStyleCoord and when we retrieve an auto value, we do
> our own auto size calculation in nsIFrame::GetMarginRectRelativeToSelf().

(I'm not sure I understand how this option would work -- once we've finished reflow, I think it'd be very hard to reverse-engineer which component of our position was determined by "margin", vs. other things like relative positioning, alignment, etc.  We can't rely on GetMarginRectRelativeToSelf because that function itself depends on the UsedMargin frame-property.)
Flags: needinfo?(dholbert)
Status: NEW → ASSIGNED
This blocks the flexbox inspector because of the way we render the justify content visualization in the highlighter makes use of this API. See https://bugzilla.mozilla.org/show_bug.cgi?id=1432029#c13.
Attachment #8961905 - Flags: review?(dholbert)
Attachment #8961906 - Flags: review?(dholbert)
Attachment #8961907 - Flags: review?(dholbert)
Attachment #8961908 - Flags: review?(dholbert)
Comment on attachment 8961905 [details]
Bug 1298008 Part 1: Update GeometryUtils::GetBoxRectForFrame to use GetMarginRectRelativeToSelf for margin boxes.

https://reviewboard.mozilla.org/r/230730/#review236834

Nice. Looks like this is basically a simplification that doesn't impact functionality (modulo possible special cases around GetSkipSides inside GetMarginRectRelativeToSelf -- but that only matters for frames that are fragmented, and that's a step in the direction of correctness).

Ideally it's nice to call out that sort of thing in the extended commit message (to say that the patch is basically a simplification of existing code & doesn't [really] change behavior).  But I don't care enough to tag it as a MozReview issue, in part because as noted above, this does *kind of* change behavior (probably for the better).
Attachment #8961905 - Flags: review?(dholbert) → review+
(Sorry, MozReview captured & posted two different revisions of my comment somehow. --> dropped one of them)
Comment on attachment 8961906 [details]
Bug 1298008 Part 2: Make nsFlexContainer update UsedMargin property after final reflow.

https://reviewboard.mozilla.org/r/230732/#review237166

> It looks like the "wiping" that you're undoing comes from this code here:
> https://dxr.mozilla.org/mozilla-central/rev/de32269720d056972b85f4eec5f0a8286de6e3af/layout/generic/ReflowInput.cpp#2573
> 
> So I think you can avoid bothering with "restoring" this frame-property if you manually modify the margin on the reflow state.  Then the "wiping" will actually wipe the correct value.
> 
> To do that, I think you need to adjust ReflowFlexItem() - its childReflowInput(...) constructor calls InitOffsets (and triggers that wiping) automatically right now, but there's another ReflowInput constructor that you can use that lets you modify some things and then call Init manually... I think that's probably what you want to do here.

Oh, hmm, I guess InitOffsets() tries to compute the margin *and* wipe the property all in one fell swoop. So there's no opportunity to insert the correct value into the ReflowInput (before the "wipe") after all.  So, disregard comment 13... Maybe the current approach is the best we can do after all.
Comment on attachment 8961907 [details]
Bug 1298008 Part 3: Update ReflowInput::CalculateBlockSideMargins to store computed values in the UsedMargin property.

https://reviewboard.mozilla.org/r/230734/#review237262

::: layout/generic/ReflowInput.cpp:2569
(Diff revision 2)
> -  // XXX We need to include 'auto' horizontal margins in this too!
> -  // ... but if we did that, we'd need to fix nsFrame::GetUsedMargin
> -  // to use it even when the margins are all zero (since sometimes
> +  // 'auto' margins are calculated as zero. Callers who want useful values for
> +  // UsedMargin property are responsible for updating that property with real
> +  // computed values. ::CalculateBlockSideMargins() contains an example of this.

I have several concerns about this wording:
(1) The current wording implies that "0" isn't a "useful" value - but really, it's the correct resolved value for 'auto' in most contexts. e.g. for margin-top/margin-bottom in a block (in default writing mode), and for all sides on an inline-block, "auto" is correctly resolved to 0.

(2) "Callers" isn't quite correct here - the fixup that you're calling for doesn't really happen in our (direct) callers, AFAICT.

How about we reword this paragraph to the following, or something like it:
  // Note that ComputeMargin() simplistically resolves 'auto' margins to 0.
  // In formatting contexts where this isn't correct, some later code will
  // need to update the UsedMargin() property with the actual resolved value.
  // One example of this is ::CalculateBlockSideMargins()
Attachment #8961907 - Flags: review?(dholbert) → review+
Comment on attachment 8961908 [details]
Bug 1298008 Part 4: Add a web-platform test of getBoxQuads on block and flex boxes with auto margins.

https://reviewboard.mozilla.org/r/230736/#review237312

::: layout/generic/test/test_computed_auto_margin.html:37
(Diff revision 2)
> +<script>
> +let bb = document.getElementById("block-block");
> +is(bb.getBoxQuads({box: "border"})[0].bounds.width, 20, "Block layout border box is expected width.");
> +is(bb.getBoxQuads({box: "margin"})[0].bounds.width, 100, "Block layout margin box is expected width.");
> +
> +let fb = document.getElementById("flex-block");
> +is(fb.getBoxQuads({box: "border"})[0].bounds.width, 20, "Flex layout border box is expected width.");
> +is(fb.getBoxQuads({box: "margin"})[0].bounds.width, 100, "Flex layout margin box is expected width.");

Please test block-axis "auto" margins, too.

Maybe:
 - In the ".container" rule, add "height:50px"
 - In the "span" rule, use "margin: auto" instead of "0 auto"
 - Check bb.getBoxQuads(...).bounds.height (not just width).

For the block child ("bb"), I think the vertical margin resolves to 0, so the margin box & border box would be the same.

For the flex child, the vertical margin should resolve to the height of the flex container - e.g. 50px if that's what you use for the container height.

(This might actually require another code-patch, too. I think your "Part 2" here handles *main-axis* 'auto' margins, but not *cross-axis* 'auto' margins yet.)
Attachment #8961908 - Flags: review?(dholbert) → review-
Comment on attachment 8961908 [details]
Bug 1298008 Part 4: Add a web-platform test of getBoxQuads on block and flex boxes with auto margins.

https://reviewboard.mozilla.org/r/230736/#review237312

> Please test block-axis "auto" margins, too.
> 
> Maybe:
>  - In the ".container" rule, add "height:50px"
>  - In the "span" rule, use "margin: auto" instead of "0 auto"
>  - Check bb.getBoxQuads(...).bounds.height (not just width).
> 
> For the block child ("bb"), I think the vertical margin resolves to 0, so the margin box & border box would be the same.
> 
> For the flex child, the vertical margin should resolve to the height of the flex container - e.g. 50px if that's what you use for the container height.
> 
> (This might actually require another code-patch, too. I think your "Part 2" here handles *main-axis* 'auto' margins, but not *cross-axis* 'auto' margins yet.)

Oh, part 2 does handle cross-axis 'auto' margins (in "SingleLineCrossAxisPositionTracker"). Nice! So probably no additional code-patch needed after all -- just tests for that cross-axis margin (the margin-box height in this case).
Comment on attachment 8961906 [details]
Bug 1298008 Part 2: Make nsFlexContainer update UsedMargin property after final reflow.

https://reviewboard.mozilla.org/r/230732/#review237318

This "part 2" approach feels like we're looking-up & setting (& re-setting) this frame property a lot.  This feels unnecessary, since we're also indepdendently storing the margin on the FlexItem.  Can we just keep track of the fact that we have an auto margin, and then (if we do), make a single stomp on the frame property after the ReflowFlexItem call?

That would do fewer hashtable lookups, and would make better use of the data that we're already keeping track of.

I'm imagining something like this:
 (1) Give the FlexItem class a new "bool mHasAnyAutoMargin" member-var, alongside the other boolean vars, with a HasAnyAutoMargin() getter.  (Probably no setter is necessary.)

 (2) Initialize this member-var in the constructor by pulling this variable up a few lines, outside of "#ifdef DEBUG":
https://dxr.mozilla.org/mozilla-central/rev/de32269720d056972b85f4eec5f0a8286de6e3af/layout/generic/nsFlexContainerFrame.cpp#1856-1858
...and then doing something like:
 mHasAnyAutoMargin = styleMargin.HasInlineAxisAuto(mWM) ||
   styleMargin.HasBlockAxisAuto(mWM);

 (3) After we've left the "if (itemNeedsReflow){ ReflowFlexItem(...); } block, do something like:
   if (item->HasAnyAutoMargin()) {
     nsMargin* propValue = item->Frame()->GetProperty(nsIFrame::UsedMarginProperty());
     if (propValue) {
       *propValue = item->GetMargin();
     }
   }

(I suspect we need to do this unconditionally -- outside the "itemNeedsReflow" check -- because even if we skip that final reflow, it's possible we did an earlier "measuring reflow" to resolve min-height:auto for example, and that measuring reflow may have stomped on our frame property.)

(This proposed "mHasAnyAutoMargin" bool could be used to optimize a few other flex item methods -- e.g. we could have an early "return 0" in FlexItem::GetNumAutoMarginsInAxis() for the common case when mHasAnyAutoMargin is false -- but that sort of optimization doesn't need to be in-scope for this bug/patch.)
Comment on attachment 8961906 [details]
Bug 1298008 Part 2: Make nsFlexContainer update UsedMargin property after final reflow.

https://reviewboard.mozilla.org/r/230732/#review237350

r=me, two nits:

::: layout/generic/nsFlexContainerFrame.cpp:1859
(Diff revision 2)
> +  const nsStyleSides& styleMargin =
> +        aFlexItemReflowInput.mStyleMargin->mMargin;

Please deindent "aFlexItemReflowInput" so that it's only 2 spaces more indented than the line above it.

::: layout/generic/nsFlexContainerFrame.cpp:1861
(Diff revision 2)
>    CheckForMinSizeAuto(aFlexItemReflowInput, aAxisTracker);
>  
> +
> +  const nsStyleSides& styleMargin =
> +        aFlexItemReflowInput.mStyleMargin->mMargin;
> +  mHasAnyAutoMargin = styleMargin.HasInlineAxisAuto(mWM) ||

We also need to initialize this bool in the other FlexItem() constructor (the "strut" constructor, for visibility:collapse elements), too:
https://dxr.mozilla.org/mozilla-central/rev/de32269720d056972b85f4eec5f0a8286de6e3af/layout/generic/nsFlexContainerFrame.cpp#1885-1889

You can just set it to false in the init list there, below "mNeedsMinSizeAutoResolution(false),":
https://dxr.mozilla.org/mozilla-central/rev/de32269720d056972b85f4eec5f0a8286de6e3af/layout/generic/nsFlexContainerFrame.cpp#1917

These "struts" don't really participate in layout as much as real flex items (they just impose a minimum cross-size on their flex line). So we don't need to bother giving auto margins special handling there.
Attachment #8961906 - Flags: review?(dholbert) → review+
Comment on attachment 8961908 [details]
Bug 1298008 Part 4: Add a web-platform test of getBoxQuads on block and flex boxes with auto margins.

https://reviewboard.mozilla.org/r/230736/#review237352

::: layout/generic/test/test_computed_auto_margin.html:43
(Diff revision 3)
> +<script>
> +let bb = document.getElementById("block-block");
> +is(bb.getBoxQuads({box: "border"})[0].bounds.width,  20, "Block layout border box is expected width.");
> +is(bb.getBoxQuads({box: "margin"})[0].bounds.width, 100, "Block layout margin box is expected width.");
> +
> +// For containers that expand items to fill block space, measure the box heights also.

s/block space/block-axis space/

("block space" is a bit confusing, since this is the *non-block-container* scenario. :))
Attachment #8961908 - Flags: review?(dholbert) → review+
Comment on attachment 8961908 [details]
Bug 1298008 Part 4: Add a web-platform test of getBoxQuads on block and flex boxes with auto margins.

https://reviewboard.mozilla.org/r/230736/#review237354

Sorry for the drive-by comment, feel free to not address it if you don't want to.

::: layout/generic/test/test_computed_auto_margin.html:40
(Diff revision 3)
> +<span id="flex-block"></span>
> +</div>
> +
> +<script>
> +let bb = document.getElementById("block-block");
> +is(bb.getBoxQuads({box: "border"})[0].bounds.width,  20, "Block layout border box is expected width.");

Can this be a wpt test? That'd help every engine :)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #27)
> Comment on attachment 8961908 [details]
> Can this be a wpt test? That'd help every engine :)

Good point; I didn't realize that getBoxQuads() was part of the spec. I'll change this to a submitted web-platform test.
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40f53976a5b8
Part 1: Update GeometryUtils::GetBoxRectForFrame to use GetMarginRectRelativeToSelf for margin boxes. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/b5a4113f4649
Part 2: Make nsFlexContainer update UsedMargin property after final reflow. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c18e2aac48a5
Part 3: Update ReflowInput::CalculateBlockSideMargins to store computed values in the UsedMargin property. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/f5d5d6a09965
Part 4: Add a web-platform test of getBoxQuads on block and flex boxes with auto margins. r=dholbert
What happened here?  It was backed out from autoland but then merged to central?  I guess central is still getting the backout... right?
Flags: needinfo?(nbeleuzu)
Sorry :jryans, my fault. I did another merge from autoland to m-c in order to fix
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=8059b8b2abe9aee49ae69c804b8d9fef6ae42df5
Flags: needinfo?(nbeleuzu)
Flags: needinfo?(bwerth)
Status: RESOLVED → REOPENED
status-firefox61: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla61 → ---
(Seems like :ni about failure is worth keeping.)
Flags: needinfo?(bwerth)
Attachment #8964306 - Flags: review?(dholbert)
Attachment #8964307 - Flags: review?(gl)
Comment on attachment 8964307 [details]
Bug 1298008 Part 6: Update a devtools test that checks the computed values of auto margins in the box model.

https://reviewboard.mozilla.org/r/233026/#review238500
Attachment #8964307 - Flags: review?(gl) → review+
Comment on attachment 8964306 [details]
Bug 1298008 Part 5: Change a web-platform test that checks getComputedStyle to PASS.

https://reviewboard.mozilla.org/r/233024/#review238502
Attachment #8964306 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Flags: needinfo?(bwerth)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f8977bb0471f
Part 1: Update GeometryUtils::GetBoxRectForFrame to use GetMarginRectRelativeToSelf for margin boxes. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/94c0105f79d0
Part 2: Make nsFlexContainer update UsedMargin property after final reflow. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/3dd522fee1ea
Part 3: Update ReflowInput::CalculateBlockSideMargins to store computed values in the UsedMargin property. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c37ba131291e
Part 4: Add a web-platform test of getBoxQuads on block and flex boxes with auto margins. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/3b3b14692cc4
Part 5: Change a web-platform test that checks getComputedStyle to PASS. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/55602e9462ae
Part 6: Update a devtools test that checks the computed values of auto margins in the box model. r=gl
Keywords: checkin-needed
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10313 for changes under testing/web-platform/tests
Whiteboard: [designer-tools] → [designer-tools][wptsync upstream]
Whiteboard: [designer-tools][wptsync upstream] → [designer-tools][wptsync upstream error]
Whiteboard: [designer-tools][wptsync upstream error] → [designer-tools][wptsync upstream]
Whiteboard: [designer-tools][wptsync upstream] → [designer-tools][wptsync upstream error]
Whiteboard: [designer-tools][wptsync upstream error] → [designer-tools][wptsync upstream]
Depends on: 1488080
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: