Last Comment Bug 718521 - Incorrect clipping of multiple items using rounded rectangles
: Incorrect clipping of multiple items using rounded rectangles
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on:
Blocks: GPU-clipping-rounded
  Show dependency treegraph
 
Reported: 2012-01-16 14:22 PST by Nick Cameron [:nrc]
Modified: 2012-01-19 17:44 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Roll the mouse over the top video to see transient incorrect clipping of the bottom video (675 bytes, text/html)
2012-01-16 14:22 PST, Nick Cameron [:nrc]
no flags Details
another test case (813 bytes, text/html)
2012-01-16 16:58 PST, Nick Cameron [:nrc]
no flags Details
proposed patch (not yet properly tested) (2.90 KB, patch)
2012-01-16 17:08 PST, Nick Cameron [:nrc]
roc: review+
Details | Diff | Review
revised patch (2.75 KB, patch)
2012-01-16 18:14 PST, Nick Cameron [:nrc]
no flags Details | Diff | Review
reftest for the bugfix (4.36 KB, patch)
2012-01-16 19:47 PST, Nick Cameron [:nrc]
no flags Details | Diff | Review
updated reftest (3.30 KB, patch)
2012-01-16 20:26 PST, Nick Cameron [:nrc]
no flags Details | Diff | Review
another go at the reftest (3.07 KB, patch)
2012-01-17 16:22 PST, Nick Cameron [:nrc]
no flags Details | Diff | Review
updated reftest (2.93 KB, patch)
2012-01-17 17:35 PST, Nick Cameron [:nrc]
roc: review+
Details | Diff | Review
added fuzzing to the reftest (2.93 KB, patch)
2012-01-18 17:39 PST, Nick Cameron [:nrc]
no flags Details | Diff | Review
updated reftest (2.92 KB, patch)
2012-01-18 18:11 PST, Nick Cameron [:nrc]
no flags Details | Diff | Review

Description Nick Cameron [:nrc] 2012-01-16 14:22:59 PST
Created attachment 589012 [details]
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 [:bz] 2012-01-16 15:50:09 PST
Hmm.  So the incorrect clipping appears when the video controls are appearing or disappearing.  I wonder why...
Comment 2 Nick Cameron [:nrc] 2012-01-16 15:52:33 PST
It is a bug in the layers code. Patch is almost ready...
Comment 3 Nick Cameron [:nrc] 2012-01-16 16:58:12 PST
Created attachment 589066 [details]
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) (Exited; email my personal email if necessary) 2012-01-16 16:59:43 PST
Cool, that will make a good reftest.
Comment 5 Nick Cameron [:nrc] 2012-01-16 17:08:40 PST
Created attachment 589069 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2012-01-16 17:21:45 PST
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] 2012-01-16 18:14:07 PST
Created attachment 589080 [details] [diff] [review]
revised patch

removed assertion comments
Comment 8 Nick Cameron [:nrc] 2012-01-16 18:51:05 PST
I have a reftest, any suggestions for which directory it belongs in?
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-16 19:35:04 PST
probably just layout/reftest/bugs
Comment 10 Nick Cameron [:nrc] 2012-01-16 19:47:47 PST
Created attachment 589089 [details] [diff] [review]
reftest for the bugfix
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-16 19:54:21 PST
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] 2012-01-16 20:26:42 PST
Created attachment 589098 [details] [diff] [review]
updated reftest

addressed roc's review - much simpler test
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-16 20:31:30 PST
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] 2012-01-17 16:22:36 PST
Created attachment 589339 [details] [diff] [review]
another go at the reftest
Comment 15 Nick Cameron [:nrc] 2012-01-17 16:24:30 PST
(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) (Exited; email my personal email if necessary) 2012-01-17 16:48:42 PST
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] 2012-01-17 17:35:19 PST
Created attachment 589372 [details] [diff] [review]
updated reftest

Using divs rather than canvases in the ref
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-18 04:06:53 PST
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] 2012-01-18 17:39:19 PST
Created attachment 589732 [details] [diff] [review]
added fuzzing to the reftest

added fuzzing to the reftest and polished the HTML files a little
Comment 20 Nick Cameron [:nrc] 2012-01-18 18:11:08 PST
Created attachment 589744 [details] [diff] [review]
updated reftest

Turns out I need fuzzing for windows too
Comment 21 Mozilla RelEng Bot 2012-01-18 20:00:55 PST
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/cpearce@mozilla.com-c26e7352a943
Comment 22 Mozilla RelEng Bot 2012-01-18 22:30:50 PST
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/cpearce@mozilla.com-b082b49340a2

Note You need to log in before you can comment on or make changes to this bug.