The default bug view has changed. See this FAQ.

Incorrect clipping of multiple items using rounded rectangles

RESOLVED FIXED in mozilla12

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

Trunk
mozilla12
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Component: Graphics → Layout
QA Contact: thebes → layout
Hmm.  So the incorrect clipping appears when the video controls are appearing or disappearing.  I wonder why...
Assignee: ncameron → nobody
Component: Layout → Video/Audio
QA Contact: layout → video.audio
(Assignee)

Comment 2

5 years ago
It is a bug in the layers code. Patch is almost ready...
Component: Video/Audio → Layout
(Assignee)

Updated

5 years ago
Assignee: nobody → ncameron
(Assignee)

Comment 3

5 years ago
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.
Cool, that will make a good reftest.
(Assignee)

Comment 5

5 years ago
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.
Attachment #589069 - Flags: feedback?(roc)
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
Attachment #589069 - Flags: feedback?(roc) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 589080 [details] [diff] [review]
revised patch

removed assertion comments
Attachment #589069 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
I have a reftest, any suggestions for which directory it belongs in?
probably just layout/reftest/bugs
(Assignee)

Comment 10

5 years ago
Created attachment 589089 [details] [diff] [review]
reftest for the bugfix
Attachment #589089 - Flags: review?(roc)
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.
(Assignee)

Comment 12

5 years ago
Created attachment 589098 [details] [diff] [review]
updated reftest

addressed roc's review - much simpler test
Attachment #589089 - Attachment is obsolete: true
Attachment #589089 - Flags: review?(roc)
Attachment #589098 - Flags: review?(roc)
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.
(Assignee)

Comment 14

5 years ago
Created attachment 589339 [details] [diff] [review]
another go at the reftest
Attachment #589098 - Attachment is obsolete: true
Attachment #589098 - Flags: review?(roc)
Attachment #589339 - Flags: review?(roc)
(Assignee)

Comment 15

5 years ago
(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 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?
(Assignee)

Comment 17

5 years ago
Created attachment 589372 [details] [diff] [review]
updated reftest

Using divs rather than canvases in the ref
Attachment #589339 - Attachment is obsolete: true
Attachment #589339 - Flags: review?(roc)
Attachment #589372 - Flags: review?(roc)
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.
Attachment #589372 - Flags: review?(roc) → review+
(Assignee)

Comment 19

5 years ago
Created attachment 589732 [details] [diff] [review]
added fuzzing to the reftest

added fuzzing to the reftest and polished the HTML files a little
Attachment #589372 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 589744 [details] [diff] [review]
updated reftest

Turns out I need fuzzing for windows too
Attachment #589732 - Attachment is obsolete: true

Comment 21

5 years ago
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

5 years ago
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/55168447493a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0460a3d6ca3b
https://hg.mozilla.org/mozilla-central/rev/55168447493a
https://hg.mozilla.org/mozilla-central/rev/0460a3d6ca3b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.