Closed
Bug 1424673
Opened 6 years ago
Closed 6 years ago
Banner ad's cloud images are positioned incorrectly with WebRender enabled
Categories
(Core :: Graphics: WebRender, defect, P1)
Core
Graphics: WebRender
Tracking
()
VERIFIED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | --- | verified |
firefox60 | --- | verified |
People
(Reporter: cpeterson, Assigned: ethlin)
References
Details
(Keywords: regression, Whiteboard: [wr-mvp] [triage])
Attachments
(5 files)
The attached banner ad should show white clouds on a blue background, but instead shows jumbled white boxes and clouds when WebRender is enabled. I've attached an HTML file of ad that loads the remote files. I'll attach zip file with the complete HTML and assets saved locally for posterity.
Reporter | ||
Comment 1•6 years ago
|
||
This zip file contains the complete HTML and assets saved locally.
Reporter | ||
Comment 2•6 years ago
|
||
Here is a screenshot.
Reporter | ||
Comment 3•6 years ago
|
||
This bug is a regression from WebRender update bug 1417062. Perhaps this bug is a dupe of bug 1421526 or bug 1421272, other rendering regressions from the same WebRender update.
Blocks: 1417062
status-firefox57:
--- → unaffected
status-firefox58:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Keywords: regression
Updated•6 years ago
|
Whiteboard: [wr-mvp] [triage]
Updated•6 years ago
|
Blocks: stage-wr-trains
Priority: -- → P1
Comment 4•6 years ago
|
||
WR isn't shipping in 59, right? Assuming that's correct, marking 59:fix-optional.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ethlin
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
try result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db6c62ff95963793804815561f70cbcfcb511fb3
Comment 8•6 years ago
|
||
the try push linked in comment 7 doesn't have the reftest. The try push linked from mozreview does have the reftest, and it's failing. I verified locally by loading the test and reference images in 58 beta (non-WR) and I get different results there too, so the reftest seems to be wrong. I also don't really understand the patch or the reftest, so it would help if you explained what the problem is in more detail.
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8) > the try push linked in comment 7 doesn't have the reftest. The try push > linked from mozreview does have the reftest, and it's failing. I verified > locally by loading the test and reference images in 58 beta (non-WR) and I > get different results there too, so the reftest seems to be wrong. > > I also don't really understand the patch or the reftest, so it would help if > you explained what the problem is in more detail. I only checked locally before pushing to try. I'll rewrite the reftest. For the reftest, I just simplified the original clound.html, so actually I'm not sure how to use a simpler way to write the reftest. About the clip region, we don't need to push the display item's clip manually since the ScrollingLayersHelper will do the clipping things. But we push text's webrender commands under a draw target, so we should respect the draw target boundaries. For the text draw target, the clip region should be same as the boundaries. The 1 pixel inflation is copied from [1]. We need to 1 pixel inflation or we'll fail some reftests. [1] https://dxr.mozilla.org/mozilla-central/rev/c38d22170d6f27e94c3c53093215d20255fab27a/layout/generic/nsTextFrame.cpp#5151
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•6 years ago
|
||
The new try result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e48e88556ade0f1d94a9decdd5fd84a529d02f7e
Comment 13•6 years ago
|
||
(In reply to Ethan Lin[:ethlin] from comment #9) > About the clip region, we don't need to push the display item's clip > manually since the ScrollingLayersHelper will do the clipping things. But we > push text's webrender commands under a draw target, so we should respect the > draw target boundaries. For the text draw target, the clip region should be > same as the boundaries. The 1 pixel inflation is copied from [1]. We need to > 1 pixel inflation or we'll fail some reftests. Thanks, this makes sense.
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8940919 [details] Bug 1424673 - Fix the clip region for text draw target. https://reviewboard.mozilla.org/r/211188/#review217490
Attachment #8940919 -
Flags: review?(bugmail) → review+
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8940920 [details] Bug 1424673 - Add a reftest for the bug. https://reviewboard.mozilla.org/r/211190/#review217494 ::: gfx/tests/reftest/1424673.html:24 (Diff revision 2) > + var b1 = "<div class=\"text_shadow\" style=\"position: absolute; top: "; > + document.write(b1 + "34px; left: 28px;\">.<\/div>"); > + document.write(b1 + "46px; left: 0px;\">.<\/div>"); > + document.write(b1 + "24px; left: 0px;\">.<\/div>"); You can avoid all the backslashes by using single quotes instead of double quotes, for either the JS expression or the HTML attributes. Also you shouldn't need the backslashes in <\/div> ::: gfx/tests/reftest/1424673.html:33 (Diff revision 2) > + var a1 = "<div style=\"position: relative; top: "; > + var a2 = "px; left: "; > + var a3 = "px;\"><scr"+"ipt type=\"text\/javascript\">shadow();<\/script><\/div>"; > + for(i = 0; i < 3; i++) { > + document.write(a1 + top + a2 + left + a3); Consider using template literals instead, it will probably make this cleaner. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals document.write(`<div style="position: relative; top: ${top}px; left: ${left}px"><scr` + `ipt type="text/javascript">shadow()</script></div>`);
Attachment #8940920 -
Flags: review?(bugmail) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•6 years ago
|
||
I removed all variables and loop in the test case. I think it's cleaner now. try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a833660ef2ab4aec1ff80f407133ad925fa6aa77
Comment 18•6 years ago
|
||
Pushed by ethlin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/baa4d5ed8cfd Fix the clip region for text draw target. r=kats https://hg.mozilla.org/integration/autoland/rev/2ae7cff03727 Add a reftest for the bug. r=kats
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/baa4d5ed8cfd https://hg.mozilla.org/mozilla-central/rev/2ae7cff03727
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•6 years ago
|
QA Whiteboard: [good first verify]
Comment 21•6 years ago
|
||
I successfully reproduced this issue on Firefox Nightly 59.0a1 (2017-12-10) under macOS 10.12.6 using the attachment from Comment 0. The bug is no longer reproducible on Firefox 59 RC and Nightly 60.0a1 (2018-12-03) under Windows 10 (x64), Ubuntu 16.04.3 (x64) and macOS 10.12.6.
You need to log in
before you can comment on or make changes to this bug.
Description
•