Closed Bug 1031726 Opened 6 years ago Closed 6 years ago

Layout of clip:rect() is wrong when fragments is involved

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox30 --- unaffected
firefox31 --- unaffected
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(2 files, 2 obsolete files)

Attached file Testcase #1
Load the attached testcase in Print Preview mode.
Note that on the second page the block with the dashed blue
border is clipped on the top side.  It shouldn't be, because
the top side was clipped on the first fragment on the first
page.  What we do now is more like what we would want for
box-decoration-break:clone, but for slice we want to clip
the area of the joined fragments.

Fwiw, Chrome seems to have the correct layout.
Blocks: 988659
Assignee: nobody → mats
Attached patch slice-auto-bottom (obsolete) — Splinter Review
Regarding the bottom='auto' case for box-decoration-break:'slice'.
I had this also at first, for symmetry reasons, but I don't think
it's needed.  An abs.pos. is split exactly at the fragmentainer
edge, so its overflowing children, must be clipped there too.
I can't think of a case where the abs.pos. is clipped at Y,
but its children should be clipped at Y+n (n>0).
Attached patch fix+tests, v2Splinter Review
I missed the b2g-only failure on my Try run, sorry about that.

This is simpler than I first thought, we just need to offset
the clip rect here, nothing else.  (I've added a test that fails
on all platforms with the previous patch.)

Completely green on Try this time ;-)
https://tbpl.mozilla.org/?tree=Try&rev=b34f69e63085
https://tbpl.mozilla.org/?tree=Try&rev=2a0046a05e2b
Attachment #8447929 - Attachment is obsolete: true
Attachment #8449608 - Flags: review?(roc)
Flags: needinfo?(mats)
Comment on attachment 8449608 [details] [diff] [review]
fix+tests, v2

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

::: layout/generic/nsFrame.cpp
@@ +1627,5 @@
> +    nscoord y = 0;
> +    for (nsIFrame* f = GetPrevContinuation(); f; f = f->GetPrevContinuation()) {
> +      y += f->GetRect().height;
> +    }
> +    aRect->MoveBy(nsPoint(0, -y));

You can slightly simplify this by eliminating y and just putting aRect->MoveBy(nsPoint(0, f->GetRect().height)) in the loop.
Attachment #8449608 - Flags: review?(roc) → review+
Summary: Layout of clipt:rect() is wrong when fragments is involved → Layout of clip:rect() is wrong when fragments is involved
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> > +    nscoord y = 0;
> > +    for (nsIFrame* f = GetPrevContinuation(); f; f = f->GetPrevContinuation()) {
> > +      y += f->GetRect().height;
> > +    }
> > +    aRect->MoveBy(nsPoint(0, -y));
> 
> You can slightly simplify this by eliminating y and just putting
> aRect->MoveBy(nsPoint(0, f->GetRect().height)) in the loop.

I'm aware of that; I prefer the above pattern since it allows 'y'
to be placed in a register, and I think it's also slightly easier
to understand since it separates calculating the offset (and gives
it a name) from applying the offset.  In this case it also makes
the unary minus stand out better when reading.

(Granted, performance doesn't matter much here).
https://hg.mozilla.org/mozilla-central/rev/9e4399a72511
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Blocks: 1036400
No longer blocks: 1036400
Comment on attachment 8449608 [details] [diff] [review]
fix+tests, v2

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 (bug 1025669) and clip:rect()
(this bug) 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 #8449608 - Flags: approval-mozilla-aurora?
Attachment #8447931 - Attachment is obsolete: true
> (The request is for all the patches in this bug.)

Actually, the one patch.
Comment on attachment 8449608 [details] [diff] [review]
fix+tests, v2

Aurora+
Attachment #8449608 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.