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)

defect

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)

Attached file clouds.html
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.
This zip file contains the complete HTML and assets saved locally.
Attached image clouds-screensnot.png
Here is a screenshot.
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.
Whiteboard: [wr-mvp] [triage]
WR isn't shipping in 59, right? Assuming that's correct, marking 59:fix-optional.
Assignee: nobody → ethlin
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.
(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
(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 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 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+
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
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
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
QA Whiteboard: [good first verify]
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: