Closed
Bug 718521
Opened 12 years ago
Closed 12 years ago
Incorrect clipping of multiple items using rounded rectangles
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: nrc, Assigned: nrc)
References
Details
Attachments
(4 files, 6 obsolete files)
675 bytes,
text/html
|
Details | |
813 bytes,
text/html
|
Details | |
2.75 KB,
patch
|
Details | Diff | Splinter Review | |
2.92 KB,
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
Component: Graphics → Layout
QA Contact: thebes → layout
Comment 1•12 years ago
|
||
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•12 years ago
|
||
It is a bug in the layers code. Patch is almost ready...
Component: Video/Audio → Layout
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → ncameron
Assignee | ||
Comment 3•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
removed assertion comments
Attachment #589069 -
Attachment is obsolete: true
Assignee | ||
Comment 8•12 years ago
|
||
I have a reftest, any suggestions for which directory it belongs in?
probably just layout/reftest/bugs
Assignee | ||
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Attachment #589098 -
Attachment is obsolete: true
Attachment #589098 -
Flags: review?(roc)
Attachment #589339 -
Flags: review?(roc)
Assignee | ||
Comment 15•12 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•12 years ago
|
||
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•12 years ago
|
||
added fuzzing to the reftest and polished the HTML files a little
Attachment #589372 -
Attachment is obsolete: true
Assignee | ||
Comment 20•12 years ago
|
||
Turns out I need fuzzing for windows too
Attachment #589732 -
Attachment is obsolete: true
Comment 21•12 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•12 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
Comment 23•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/55168447493a https://hg.mozilla.org/integration/mozilla-inbound/rev/0460a3d6ca3b
Comment 24•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/55168447493a https://hg.mozilla.org/mozilla-central/rev/0460a3d6ca3b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in
before you can comment on or make changes to this bug.
Description
•