718521 - Incorrect clipping of multiple items using rounded rectangles

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Nick Cameron [:nrc]]

Attachment: Roll the mouse over the top video to see transient incorrect clipping of the bottom video

When multiple items are clipped (with a rounded rectangle) by a single container, items other than the first are incorrectly clipped, that is, without the rounding on the rectangle.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  So the incorrect clipping appears when the video controls are appearing or disappearing.  I wonder why...

Comment 2

[Nick Cameron [:nrc]]

It is a bug in the layers code. Patch is almost ready...

Comment 3

[Nick Cameron [:nrc]]

Attachment: another test case

With three videos, rather than two, you don't need to roll your mouse over a video to cause an error.

Comment 4

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

Cool, that will make a good reftest.

Comment 5

[Nick Cameron [:nrc]]

Attachment: proposed patch (not yet properly tested)

The patch solves the bug as found in the two test cases, but I haven't run it through regression testing yet, nor created a reftest.

Comment 6

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

Comment on attachment 589069 [details] [diff] [review]
proposed patch (not yet properly tested)

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

This can land once you have a reftest

::: layout/base/FrameLayerBuilder.cpp
@@ +2365,5 @@
>  nsRect
>  FrameLayerBuilder::Clip::NonRoundedIntersection() const
>  {
> +  //NS_ASSERTION(!mRoundedClipRects.IsEmpty(), "no rounded clip rects?");
> +    //if there are no rounded rects, just return a copy of mClipRect

Just remove the assertion

Comment 7

[Nick Cameron [:nrc]]

Attachment: revised patch

removed assertion comments

Comment 8

[Nick Cameron [:nrc]]

I have a reftest, any suggestions for which directory it belongs in?

Comment 9

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

probably just layout/reftest/bugs

Comment 10

[Nick Cameron [:nrc]]

Attachment: reftest for the bugfix

Comment 11

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

Comment on attachment 589089 [details] [diff] [review]
reftest for the bugfix

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

::: layout/reftests/bugs/718521.html
@@ +33,5 @@
> +<script type="text/javascript">
> + function draw() {
> +    drawShapes(document.getElementById('nrcCanvas0').getContext('2d'));
> +    drawShapes(document.getElementById('nrcCanvas1').getContext('2d'));
> +    drawShapes(document.getElementById('nrcCanvas2').getContext('2d'));

you can simplify the code by passing the canvas ID to drawShapes and doing getElementById/getContext in there.

@@ +37,5 @@
> +    drawShapes(document.getElementById('nrcCanvas2').getContext('2d'));
> + }
> + function drawShapes(nrcContext0) {
> +    var img = new Image();
> +    img.src = 'http://mozcom-cdn.mozilla.net/img/covehead/firefox/brand-toolkit/identity-guidelines-logo.png'; 

Big no-no --- tests must not refer to external resources :-).

Use an image that's already in the bugs directory.

@@ +43,5 @@
> +      nrcContext0.fillStyle = '#008000';
> +      nrcContext0.fillRect(0, 0, 200, 200);
> +      nrcContext0.fillStyle = '#000080';
> +      nrcContext0.fillRect(50, 50, 200, 100);
> +      nrcContext0.drawImage(img, 100, 100);

Any reason why you need to draw all this? Wouldn't a single fillRect suffice?

@@ +59,5 @@
> +  Your browser does not support the canvas tag.
> +</canvas>
> +<canvas id="nrcCanvas2" width="320" height="260">
> +  Your browser does not support the canvas tag.
> +</canvas> 

The fallback text is not needed.

Comment 12

[Nick Cameron [:nrc]]

Attachment: updated reftest

addressed roc's review - much simpler test

Comment 13

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

Comment on attachment 589098 [details] [diff] [review]
updated reftest

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

::: layout/reftests/bugs/718521-ref.html
@@ +14,5 @@
> +.clipround {
> +	border-radius:45px;
> +}
> +</style>
> +<script type="text/javascript">

type not needed

@@ +38,5 @@
> +<div class="clipround">
> +<canvas id="nrcCanvas1" width="1320" height="260"></canvas> 
> +</div>
> +<div class="clipround">
> +<canvas id="nrcCanvas2" width="320" height="1260"></canvas> 

You don't really need canvases here for the reference, you could just use divs with background colors.

::: layout/reftests/bugs/718521.html
@@ +29,5 @@
> +  left:0px;
> +  top:240px;
> +}
> +</style>
> +<script type="text/javascript">

'type' not needed.

@@ +42,5 @@
> +    ctxt.fillRect(0, 0, 300, 230);
> +  }
> + </script>
> +</head>
> +<body onload="draw();">

This doesn't have to be onload. Just put the script at the end of the document (before the body close tag) and run it synchronously. That's a little simpler and faster.

Comment 14

[Nick Cameron [:nrc]]

Attachment: another go at the reftest

Comment 15

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)

> @@ +38,5 @@
> > +<div class="clipround">
> > +<canvas id="nrcCanvas1" width="1320" height="260"></canvas> 
> > +</div>
> > +<div class="clipround">
> > +<canvas id="nrcCanvas2" width="320" height="1260"></canvas> 
> 
> You don't really need canvases here for the reference, you could just use
> divs with background colors.
> 

I prefer to use canvases because it makes it easy to do the translation and clipping that makes the two pages match. It is much more fiddly to do this with just DIVs (unless I'm missing an obvious way to do it).

All roc's other issues are fixed.

Comment 16

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

Comment on attachment 589339 [details] [diff] [review]
another go at the reftest

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

Why not make the reference canvases divs with backgrounds, and give them the same position:absolute styling as in the testcase?

Comment 17

[Nick Cameron [:nrc]]

Attachment: updated reftest

Using divs rather than canvases in the ref

Comment 18

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

Comment on attachment 589372 [details] [diff] [review]
updated reftest

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

This could be slightly better but it's definitely good enough.

Comment 19

[Nick Cameron [:nrc]]

Attachment: added fuzzing to the reftest

added fuzzing to the reftest and polished the HTML files a little

Comment 20

[Nick Cameron [:nrc]]

Attachment: updated reftest

Turns out I need fuzzing for windows too

Comment 21

[Mozilla RelEng Bot]

Try run for c26e7352a943 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c26e7352a943
Results (out of 215 total builds):
    success: 174
    warnings: 37
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c26e7352a943

Comment 22

[Mozilla RelEng Bot]

Try run for b082b49340a2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b082b49340a2
Results (out of 213 total builds):
    exception: 1
    success: 177
    warnings: 34
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b082b49340a2

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55168447493a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0460a3d6ca3b

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/55168447493a
https://hg.mozilla.org/mozilla-central/rev/0460a3d6ca3b