1025669 - Implement layout of block margins for box-decoration-break:clone

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

Attachment: Testcase #1 (from bug 998535 with margins added)

Comment 1

[Mats Palmgren (inactive)]

Attachment: wip1

Not quite working yet though:
https://tbpl.mozilla.org/?tree=Try&rev=511430382f16

REFTEST TEST-UNEXPECTED-FAIL:
layout/reftests/w3c-css/submitted/css21/pagination/moz-css21-float-page-break-inside-avoid-1.html
layout/reftests/w3c-css/submitted/css21/pagination/moz-css21-float-page-break-inside-avoid-3.html
layout/reftests/w3c-css/submitted/css21/pagination/moz-css21-float-page-break-inside-avoid-4.html
layout/reftests/bugs/563584-1.html
layout/reftests/bugs/563584-2.html
layout/reftests/bugs/563584-3.html
layout/reftests/bugs/563584-4.html
layout/reftests/printing/626395-2c.html
layout/reftests/printing/626395-2d.html
layout/reftests/printing/652178-1.html

Comment 2

[Mats Palmgren (inactive)]

Attachment: Screenshot of Testcase #1 with wip1 applied (this is the expected rendering)

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 1, Implement layout of block margins for box-decoration-break:clone

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 2, some minor cleanup

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 3, reftest

Comment 6

[Mats Palmgren (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=c382dfc61b7d
https://tbpl.mozilla.org/?tree=Try&rev=2a2c8518d96c

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8442874 [details] [diff] [review]
part 1, Implement layout of block margins for box-decoration-break:clone

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

::: layout/generic/nsBlockFrame.cpp
@@ +2930,5 @@
>  
>    // Clear past floats before the block if the clear style is not none
>    aLine->SetBreakTypeBefore(breakType);
>  
> +  bool applyTopMargin = ShouldApplyTopMargin(aState, aLine, frame);

This doesn't seem quite right. You're discarding the GetPrevInFlow check. Shouldn't we still be checking that somewhere, for the NS_STYLE_BOX_DECORATION_BREAK_SLICE case?

::: layout/generic/nsBlockReflowState.cpp
@@ +30,5 @@
> +    aMargin->top = 0;
> +  if (aSkipSides & (1 << NS_SIDE_RIGHT))
> +    aMargin->right = 0;
> +  if (aSkipSides & (1 << NS_SIDE_BOTTOM))
> +    aMargin->bottom = 0;

nsIFrame::ApplySkipSides should call this too (so we have to put it somewhere shared, maybe in BaseMargin or nsMargin?)

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 0, Add BaseMargin::ApplySkipSides, remove nsIFrame::Apply*SkipSides, and change call sites to use the former

I removed the nsIFrame methods since they don't provide much
convenience and they hide a potentially expensive GetSkipSides()
call and it seems better to have that surfaced at the call site.

(I'll file a separate bug on using a more appropriate type than
'int' for the SkipSides value.)

Comment 9

[Mats Palmgren (inactive)]

Attachment: part 1, Implement layout of block margins for box-decoration-break:clone

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> > +  bool applyTopMargin = ShouldApplyTopMargin(aState, aLine, frame);
> 
> This doesn't seem quite right. You're discarding the GetPrevInFlow check.
> Shouldn't we still be checking that somewhere, for the
> NS_STYLE_BOX_DECORATION_BREAK_SLICE case?

Yes, good catch.  I thought ShouldApplyTopMargin had that covered
somehow, with the GetSkipSides() (which checks it) affecting the
nsBlockReflowState flags in the ctor, but it's not.

I'm doing the simplest possible change here to minimize risk -
only skip the GetPrevInFlow() check in the CLONE case.  (We should
try simplify all this at some point though.)

https://tbpl.mozilla.org/?tree=Try&rev=061138783368

Comment 10

[Mats Palmgren (inactive)]

Attachment: part 0.5, Add LogicalMargin::ApplySkipSides

Looks like block margins are now logical which caused some conflicts
for these patches.  So we need a LogicalMargin::ApplySkipSides too,
similar to the BasMargin::ApplySkipSides in part 0.

Comment 11

[Mats Palmgren (inactive)]

Attachment: part 1, Implement layout of block margins for box-decoration-break:clone

(updated to use logical margins)

Comment 12

[Mats Palmgren (inactive)]

Jonathan, I'd appreciate if you glance over part 1 as well to see if I got
that right.  You can compare with the earlier "part 1" which is the
physical version.

Still green on Try, fwiw:
https://tbpl.mozilla.org/?tree=Try&rev=2dc6d72b4853

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8443951 [details] [diff] [review]
part 0.5, Add LogicalMargin::ApplySkipSides

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

::: layout/generic/nsIFrame.h
@@ +2318,5 @@
>    int GetSkipSides(const nsHTMLReflowState* aReflowState = nullptr) const;
>    virtual int
>    GetLogicalSkipSides(const nsHTMLReflowState* aReflowState = nullptr) const {
>      return 0;
>    }

While you're here, could you add a comment saying what the return values of these two methods are -- that they're really a set of bit flags, and where to find the definitions of the bits they return? Thanks.

Comment 14

[Jonathan Kew [:jfkthame]]

And yes, the updated part 1 looks OK to me.

Comment 15

[Mats Palmgren (inactive)]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> While you're here, could you add a comment saying what the return values of
> these two methods are -- that they're really a set of bit flags, and where
> to find the definitions of the bits they return? Thanks.

Sure, but I'll add that comment in bug 1028460 instead to avoid having
to rephrase it.

Comment 16

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0051c5234fe5
https://hg.mozilla.org/integration/mozilla-inbound/rev/45262e0c4f4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2fd5b2defb
https://hg.mozilla.org/integration/mozilla-inbound/rev/43f6ea484ca7
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b6b2e73a65

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0051c5234fe5
https://hg.mozilla.org/mozilla-central/rev/45262e0c4f4f
https://hg.mozilla.org/mozilla-central/rev/ae2fd5b2defb
https://hg.mozilla.org/mozilla-central/rev/43f6ea484ca7
https://hg.mozilla.org/mozilla-central/rev/f2b6b2e73a65

Comment 18

[Mats Palmgren (inactive)]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> While you're here, could you add a comment saying what the return values of
> these two methods are -- that they're really a set of bit flags, and where
> to find the definitions of the bits they return? Thanks.

Bug 1028460 made the two methods use distinct types, mozilla::Sides /
mozilla::LogicalSides, and I added pointers to where to find the type
definitions.  I think it's reasonably clear now how to use the values.
And any misuse should now result in a compile error.

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 8443952 [details] [diff] [review]
part 1, Implement layout of block margins for box-decoration-break:clone

Approval Request Comment
[Feature/regressing bug #]: 
We want to enable 'box-decoration-break' by default on Aurora
(see bug 1006326) to provide an upgrade path for a property
that were removed (-moz-background-inline-policy).
Adding support for block margins (this bug) and clip:rect()
(bug 1031726) is to make the new property feature complete.

[User impact if declined]: 
Web pages that use the old property will have some element's
background image displayed slightly wrong, and there
will be one cycle without an upgrade path to fix that.

[Describe test coverage new/current, TBPL]:
It's enabled by default on m-c.
Most of the 'box-decoration-break' patches are already on
Aurora but the feature is currently disabled by default.

[Risks and why]: minor risk

[String/UUID change made/needed]: none

(The request is for all the patches in this bug.)

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8443952 [details] [diff] [review]
part 1, Implement layout of block margins for box-decoration-break:clone

Aurora+

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8442876 [details] [diff] [review]
part 2, some minor cleanup

As per Mats' request, adding approvals to all patches.

Comment 22

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/72ca6b511540
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd81079207fd
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e2480168a62
https://hg.mozilla.org/releases/mozilla-aurora/rev/98deeb8e3843
https://hg.mozilla.org/releases/mozilla-aurora/rev/917551649830