Banner ad's cloud images are positioned incorrectly with WebRender enabled

VERIFIED FIXED in Firefox 59

Status

()

defect
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: cpeterson, Assigned: ethlin)

Tracking

(Blocks 1 bug, {regression})

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 unaffected, firefox59 verified, firefox60 verified)

Details

(Whiteboard: [wr-mvp] [triage])

Attachments

(5 attachments)

(Reporter)

Description

a year ago
Posted 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.
(Reporter)

Comment 1

a year ago
This zip file contains the complete HTML and assets saved locally.
(Reporter)

Comment 2

a year ago
Here is a screenshot.
(Reporter)

Comment 3

a year 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.
Whiteboard: [wr-mvp] [triage]
WR isn't shipping in 59, right? Assuming that's correct, marking 59:fix-optional.
(Assignee)

Updated

a year ago
Assignee: nobody → ethlin
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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

a year 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)
(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

a year 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

a year 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

a year 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

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/baa4d5ed8cfd
https://hg.mozilla.org/mozilla-central/rev/2ae7cff03727
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Duplicate of this bug: 1418718
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.