1174450 - margin-left affects layout in both dimensions with vertical text

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Mats Palmgren (inactive)]

Attachment: vertical-container-child-left-margin.html

STR: load the attached testcase

ACTUAL RESULTS
There's a 100px gap to the left of, and below, the red block.

EXPECTED RESULTS
A 100px gap to the left, no gap below, the red box.
(that's what Chrome Canary renders, fwiw)

Comment 1

[Mats Palmgren (inactive)]

Jonathan, can you confirm this is a bug? and my expected results is correct?

Comment 2

[Jonathan Kew [:jfkthame]]

Yes, looks like a bug to me. Mixed-up writing modes when initializing the reflow-state, I'd guess. Maybe Simon can provide a quick fix?

Comment 3

[Mats Palmgren (inactive)]

I discovered it by accidentally fixing it while working on Grid item intrinsic sizing.
The bug is in nsLayoutUtils::IntrinsicForContainer / IntrinsicISizeOffsets.  I'm
refactoring those to support Grid layout so I can provide a fix if you want.

Comment 4

[Jonathan Kew [:jfkthame]]

Yes please, sounds good. (I'm a bit surprised we haven't been noticing this already; anyway, let's fix asap.)

I presume this will mean passing the appropriate (container's) writing mode in to nsFrame::IntrinsicISizeOffsets. While you're there, it looks like we could also clean up the AddCoord calls to use logical accessors, along the lines of

  AddCoord(styleMargin->mMargin.GetIStart(aWM), aRenderingContext, this,
           &result.hMargin, &result.hPctMargin, false);

instead of the ugly IsVertical()?... conditional expressions.

Thanks!

Comment 5

[Mats Palmgren (inactive)]

OK, I'll submit my patches for those bits here.

Comment 6

[Mats Palmgren (inactive)]

I'll submit a series of patches that takes minimal steps to avoid mistakes in
refactoring this code and to make it easier to review...

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 1 - Split IntrinsicForContainer in two parts; move the latter part to a new function, AddIntrinsicSizeOffset, which applies the padding/border/margin from the given IntrinsicISizeOffsetData

I intentionally didn't rename the variables (which are now params)
in this patch to make it easier to review.  I'll rename these in
part 2/3.  Also, I duplicated the GetAbsoluteCoord calculations
of minISize/maxISize for now - this will come off in part 5.

Comment 8

[Mats Palmgren (inactive)]

Attachment: intradiff of the big chunk of code that was moved in part 1

This might make it easier to see the changes in the moved code.

Comment 9

[Mats Palmgren (inactive)]

Attachment: part 2 - Rename most AddIntrinsicSizeOffset params to follow the aName standard.

(intentionally leaving 'min' and 'result' for now)

Comment 10

[Mats Palmgren (inactive)]

Attachment: part 3 - Rename AddIntrinsicSizeOffset local vars from xISize to xSize since the code is generic and handles both ISizes and BSizes.

Comment 11

[Mats Palmgren (inactive)]

Attachment: part 4 - Rename the remaining AddIntrinsicSizeOffset params to follow the aName standard. Also rename local var 'w' to 'size' since it may be either a width or height.

I made 'min' and 'result' local vars again since we'll increment these
from being content-box values at the start towards margin-box at the end.

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 5 - Remove the duplicated min/max calculation in AddIntrinsicSizeOffset, pass a pointer to the calculated absolute values, if any, from IntrinsicForContainer instead

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 6 - Add a doc comment for AddIntrinsicSizeOffset and some minor code style fixes

Comment 14

[Mats Palmgren (inactive)]

Attachment: part 7 - Move most of IntrinsicForContainer into a new method, IntrinsicForWM, that takes the writing-mode to use as a parameter

IntrinsicForContainer(...) is implemented as
IntrinsicForWM(aFrame->GetParent()->GetWritingMode(), ...).

(This is needed for Grid which will pass a different WM depending
on whether we're calculating the intrinsic size in the column or
row dimension (bug 1174553))

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 8 - Add a new method nsIFrame::IntrinsicBSizeOffsets

(In reply to Jonathan Kew (:jfkthame) from comment #4)
> it looks like we could also clean up the AddCoord calls to use
> logical accessors, along the lines of
>   AddCoord(styleMargin->mMargin.GetIStart(aWM), ...

We could, but we will also need a BSize variant so I kept the code
mostly intact.

Comment 16

[Mats Palmgren (inactive)]

Attachment: part 9 - Remove the nsRenderingContext* param from Intrinsic*SizeOffsets methods since it's unused

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 10 - Use the right IntrinsicISizeOffsetData for the given writing-mode (this is what fixes the error reported in this bug)

Comment 18

[Mats Palmgren (inactive)]

Attachment: part 11 - Tests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fe74c36f1ad

Comment 19

[Mats Palmgren (inactive)]

Attachment: Rollup patch with all the parts.

Comment 20

[Jonathan Kew [:jfkthame]]

Comment on attachment 8622117 [details] [diff] [review]
part 7 - Move most of IntrinsicForContainer into a new method, IntrinsicForWM, that takes the writing-mode to use as a parameter

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

::: layout/base/nsLayoutUtils.cpp
@@ +4678,5 @@
> +{
> +  // We want the size this frame will contribute to the parent's inline-size,
> +  // so we work in the parent's writing mode; but if aFrame is orthogonal to
> +  // its parent, we'll need to look at its BSize instead of min/pref-ISize.
> +  WritingMode wm = aFrame->GetParent()->GetWritingMode();

Clearly, it wouldn't make sense to ever call this for a frame that doesn't have a parent; maybe worth adding a MOZ_ASSERT(aFrame->GetParent()) to make that explicit?

Comment 21

[Jonathan Kew [:jfkthame]]

Comment on attachment 8622118 [details] [diff] [review]
part 8 - Add a new method nsIFrame::IntrinsicBSizeOffsets

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

Looks fine, except that I'd prefer to see the |orthogonal| variable renamed; see below.

::: layout/generic/nsFrame.cpp
@@ +4098,5 @@
> +{
> +  nsIFrame::IntrinsicISizeOffsetData result;
> +  WritingMode wm = aFrame->GetWritingMode();
> +  const nsStyleMargin* styleMargin = aFrame->StyleMargin();
> +  bool orthogonal = aForISize == wm.IsVertical();

The variable name |orthogonal| seems wrong here (at least, I found it confusing when reading the code). It doesn't seem at all clear what it's saying is orthogonal to what.

This flag actually represents whether we're computing offsets in the vertical axis or the horizontal, so please rename it to |verticalAxis| or something like that.

@@ +4131,5 @@
> +    result.hBorder += styleBorder->GetComputedBorderWidth(
> +      WritingMode::PhysicalSideForBlockAxis(wm, eLogicalEdgeStart));
> +    result.hBorder += styleBorder->GetComputedBorderWidth(
> +      WritingMode::PhysicalSideForBlockAxis(wm, eLogicalEdgeEnd));
> +  }

Wouldn't it be simpler to use the |verticalAxis| flag (formerly |orthogonal|) here, too? No need to go through the PhysicalSideFor... methods at all:

  const nsStyleBorder* styleBorder = aFrame->StyleBorder();
  if (verticalAxis) {
    result.hBorder += styleBorder->GetComputedBorderWidth(NS_SIDE_TOP);
    result.hBorder += ...(NS_SIDE_BOTTOM);
  } else {
    result .... (NS_SIDE_LEFT);
    result .... (NS_SIDE_RIGHT);
  }

Comment 22

[Jonathan Kew [:jfkthame]]

Comment on attachment 8622110 [details] [diff] [review]
part 1 - Split IntrinsicForContainer in two parts; move the latter part to a new function, AddIntrinsicSizeOffset, which applies the padding/border/margin from the given IntrinsicISizeOffsetData

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

::: layout/base/nsLayoutUtils.cpp
@@ +4330,5 @@
> +                       const nsStyleCoord& styleISize,
> +                       const nsStyleCoord& styleMinISize,
> +                       const nsStyleCoord& styleMaxISize,
> +                       uint32_t aFlags,
> +                       WritingMode aContainerWM)

Hmm. Looking at how you end up using this later in bug 1174546, I wonder if it'd be better to pass just an 'aVerticalAxis' flag (or maybe a PhysicalAxis value -- see enum in WritingModes.h -- rather than a bool) instead of the WritingMode here.

I think this would let you do nsLayoutUtils::MinISizeContributionForAxis instead of ...ForWM in bug 1174546, and avoid the need to construct a "synthetic writing mode" just to pass down the vertical/horizontal choice.

Comment 23

[Mats Palmgren (inactive)]

Attachment: part 9b - Make AddIntrinsicSizeOffset take a PhysicalAxis instead of a WritingMode

I figured it's easier to review this as an increment on top of the
previous patches.

Comment 24

[Mats Palmgren (inactive)]

Attachment: part 9c - Rename local var 'orthogonal' to 'verticalAxis' and simplify getting the GetComputedBorderWidth sides

Addressing the nits in comment 21.

Comment 25

[Mats Palmgren (inactive)]

Attachment: part 10b - Rename IntrinsicForWM to IntrinsicForAxis and make it take a PhysicalAxis instead of a WritingMode

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71ce695b5ca5

Comment 26

[Mats Palmgren (inactive)]

Attachment: Rollup patch with all the parts.

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 8622668 [details] [diff] [review]
part 10b - Rename IntrinsicForWM to IntrinsicForAxis and make it take a PhysicalAxis instead of a WritingMode

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

::: layout/base/nsLayoutUtils.cpp
@@ +4635,5 @@
>          nscoord h;
>          if (GetAbsoluteCoord(styleBSize, h) ||
>              GetPercentBSize(styleBSize, aFrame, h)) {
>            h = std::max(0, h - bSizeTakenByBoxSizing);
> +          

oops - a line with just spaces managed to sneak in here.

::: layout/base/nsLayoutUtils.h
@@ +1308,5 @@
>    enum {
>      IGNORE_PADDING = 0x01
>    };
> +  static nscoord IntrinsicForAxis(mozilla::PhysicalAxis aAxis,
> +                                  nsRenderingContext*  aRenderingContext,

nit: it'd be nice to fix up the alignment of the params here.

Comment 28

[Mats Palmgren (inactive)]

Fixed.  Thanks for the quick reviews!

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06f474c578d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/049abdaa341d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ab7b15d0f9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4caa417b34ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/f06082b96326
https://hg.mozilla.org/integration/mozilla-inbound/rev/57b4bcc245bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf69f521dacd
https://hg.mozilla.org/integration/mozilla-inbound/rev/98e5282a4050
https://hg.mozilla.org/integration/mozilla-inbound/rev/c671c18ebf38
https://hg.mozilla.org/integration/mozilla-inbound/rev/aef6541d904b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ea8a36fc20
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ddd689107a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/16e74ccdc8bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/966830250aa4

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06f474c578d6
https://hg.mozilla.org/mozilla-central/rev/049abdaa341d
https://hg.mozilla.org/mozilla-central/rev/0ab7b15d0f9e
https://hg.mozilla.org/mozilla-central/rev/4caa417b34ee
https://hg.mozilla.org/mozilla-central/rev/f06082b96326
https://hg.mozilla.org/mozilla-central/rev/57b4bcc245bd
https://hg.mozilla.org/mozilla-central/rev/cf69f521dacd
https://hg.mozilla.org/mozilla-central/rev/98e5282a4050
https://hg.mozilla.org/mozilla-central/rev/c671c18ebf38
https://hg.mozilla.org/mozilla-central/rev/aef6541d904b
https://hg.mozilla.org/mozilla-central/rev/f2ea8a36fc20
https://hg.mozilla.org/mozilla-central/rev/0ddd689107a4
https://hg.mozilla.org/mozilla-central/rev/16e74ccdc8bf
https://hg.mozilla.org/mozilla-central/rev/966830250aa4