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

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks 1 bug, {testcase})

Trunk
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox31 unaffected, firefox32 fixed, firefox33 fixed)

Details

Attachments

(7 attachments, 3 obsolete attachments)

3.31 KB, text/html
Details
88.50 KB, image/png
Details
1.67 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.18 KB, patch
Details | Diff | Splinter Review
18.80 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.45 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
7.97 KB, patch
Details | Diff | Splinter Review
No description provided.
Posted patch wip1 (obsolete) — Splinter Review
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
Assignee: nobody → mats
Attachment #8440418 - Attachment is obsolete: true
Attachment #8442874 - Flags: review?(roc)
Attachment #8442876 - Flags: review?(roc)
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?)
Attachment #8442874 - Flags: review?(roc) → review-
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.)
Attachment #8443783 - Flags: review?(roc)
(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
Attachment #8442874 - Attachment is obsolete: true
Attachment #8443801 - Flags: review?(roc)
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.
Attachment #8443951 - Flags: review?(jfkthame)
(updated to use logical margins)
Attachment #8443801 - Attachment is obsolete: true
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 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.
Attachment #8443951 - Flags: review?(jfkthame) → review+
And yes, the updated part 1 looks OK to me.
(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.
Flags: in-testsuite+
(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 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.)
Attachment #8443952 - Flags: approval-mozilla-aurora?
Comment on attachment 8443952 [details] [diff] [review]
part 1, Implement layout of block margins for box-decoration-break:clone

Aurora+
Attachment #8443952 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8442876 [details] [diff] [review]
part 2, some minor cleanup

As per Mats' request, adding approvals to all patches.
Attachment #8442876 - Flags: approval-mozilla-aurora+
Attachment #8442878 - Flags: approval-mozilla-aurora+
Attachment #8443783 - Flags: approval-mozilla-aurora+
Attachment #8443951 - Flags: approval-mozilla-aurora+
Blocks: 1168921
You need to log in before you can comment on or make changes to this bug.