Product image on sephora.com does not appear unless hovering over it
Categories
(Core :: Graphics: WebRender, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox74 | --- | wontfix |
firefox75 | --- | wontfix |
firefox76 | --- | wontfix |
firefox77 | --- | verified |
People
(Reporter: ke5trel, Assigned: tnikkel)
References
(Blocks 1 open bug, Regression, )
Details
(Keywords: correctness, regression)
Attachments
(6 files, 2 obsolete files)
STR:
- Visit: https://www.sephora.com/product/squalane-probiotic-gel-moisturizer-P416561
- Note the blank space for the product image.
- Hover cursor over blank space to make image appear.
Image should appear without hovering like with WebRender disabled.
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1853d9e96a0990b01d856be8cdd67b49a9101eee&tochange=476956cf7d2e926a99b0c582550c286e3d8d44eb
Regressed by Bug 1553295.
Comment 1•5 years ago
|
||
Hi, thanks for the regression range! Would you also be able to attach a copy of your about:support?
That link unfortunately just takes me to the homepage, and if I click on a different product I cannot reproduce. Is it only on that product's page this occurs, or any product on that website?
(In reply to Jamie Nicol [:jnicol] from comment #1)
Hi, thanks for the regression range! Would you also be able to attach a copy of your about:support?
That link unfortunately just takes me to the homepage, and if I click on a different product I cannot reproduce. Is it only on that product's page this occurs, or any product on that website?
Hey. I originally posted about this in the nightly builds forum. This bug happens throughout the whole Sephora website. I have attached my about:support info for you.
Comment 4•5 years ago
|
||
I can reproduce the issue on Nightly76.0a1 Windows10 if WebRender is enabled.
With the attached html,
WebRender is enabled : the page is blank.
WebRender is disabled : the page shows an image.
Comment 5•5 years ago
|
||
Updated•5 years ago
|
Comment 6•5 years ago
|
||
Weirdly, this seems to work fine for me in a debug build but not an opt one.
Comment 7•5 years ago
|
||
It looks like what's going on here is that the "display:none" makes it so that we have no mask frame which is means we don't successfully draw a mask this is supposed to cause to not mask the item but we end up using an empty mask instead.
Comment 8•5 years ago
|
||
Layers avoids this by having IsMaskResourceReady(mFrame) return false in this case.
Comment 9•5 years ago
|
||
ooh, a cross platform webrender bug in 2020, neat!
Comment 10•5 years ago
|
||
This comment explains a bit more about the situation:
https://searchfox.org/mozilla-central/rev/9c6e7500c0015a2c60be7b1b888261d95095ce27/layout/svg/nsSVGIntegrationUtils.cpp#583
We basically want to do something similar in the BuildWrMaskImage path. i.e. detect if we're not going to get a mask and don't mask if we're in the SVG case.
Comment 11•5 years ago
|
||
Comment 12•5 years ago
|
||
This seems to cleanly implement the behaviour we want, try build here to check for situations I'm not anticipating: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f75631c932e50ca5203e7113945b403930382c3f
Comment 13•5 years ago
|
||
going on leave for a bit, so some quick notes:
- the try run demonstrates a few things that break with my patch; it's too aggressive
- Alice's attached example is the great start of a reftest
- we need to clearly understand exactly under what circumstances "invalid" masks fail to "show everything" or "show nothing" (it's Weird)
interesting failing tests with my patch:
- https://searchfox.org/mozilla-central/source/layout/reftests/w3c-css/submitted/masking/mask-image-4a.html
- https://searchfox.org/mozilla-central/source/layout/reftests/w3c-css/submitted/masking/mask-image-4b.html
links to some context on how invalid masks are Weird:
Comment 14•5 years ago
|
||
Comment 15•5 years ago
|
||
At some point in one of yesterday's builds the Sephora website started working correctly again and images are now displaying as they should.
Comment 16•5 years ago
|
||
The wpt reftest hangs. It seems to be looping because isMozAfterPaintPending stays true when we have a mask that is display:none
isMozAfterPaintPending
https://searchfox.org/mozilla-central/rev/4e228dc5f594340d35da7453829ad9f3c3cb8b58/testing/marionette/listener.js#1815
Comment 17•5 years ago
|
||
So the infinite paints are caused in part by synchronous image decoding.
Assignee | ||
Comment 18•5 years ago
|
||
I don't think any image decoding is happening, just the effect of the flag. A couple guesses for places to look
https://searchfox.org/mozilla-central/rev/4e228dc5f594340d35da7453829ad9f3c3cb8b58/layout/painting/nsDisplayList.cpp#9901
https://searchfox.org/mozilla-central/rev/4e228dc5f594340d35da7453829ad9f3c3cb8b58/layout/painting/RetainedDisplayListBuilder.cpp#74
Assignee | ||
Comment 19•5 years ago
|
||
We make the draw result NOT_READY here
https://searchfox.org/mozilla-central/rev/9120151ddb35f2d4c37bfe613a54a4f10a9a3dc5/layout/svg/nsSVGIntegrationUtils.cpp#509
if there is no frame for the mask (it's display none), which means sync decode keeps trying to invalidate to try to get a successful draw.
Assignee | ||
Comment 20•5 years ago
|
||
Spun off the infinite paints in the test issue into bug 1628988 (with proper diagnosis and hacky patch).
Reporter | ||
Comment 21•5 years ago
|
||
(In reply to tnowens from comment #15)
At some point in one of yesterday's builds the Sephora website started working correctly again and images are now displaying as they should.
Not the case for me in latest Nightly, behavior is still the same.
Assignee | ||
Comment 22•5 years ago
|
||
PaintMaskSurface shouldn't be applying ImgDrawResult::NOT_READY when we don't have a frame and the mask image hasn't been resolved. ImgDrawResult is only about drawing images, not about waiting for external resources to resolve or frames to get constructed. The only purpose of tracking ImgDrawResult's in painting code is to know which frames we need to invalidate because their rendering might change if we sync decode images during a Draw call. Applying NOT_READY here means we invalidate for every paint with the sync decode images flag (ie reftest paints), and it never changes from NOT_READY. This bites the reftest for this bug 1624532.
To fix it, instead of "overloading" the ImgDrawResult we return a bool to indicate the mask is missing or incomplete.
Comment 23•5 years ago
|
||
Comment on attachment 9139917 [details]
Bug 1624532. Don't apply ImgDrawResult::NOT_READY in PaintMaskSurface. r?mstange
Revision D70595 was moved to bug 1628988. Setting attachment 9139917 [details] to obsolete.
Assignee | ||
Comment 24•5 years ago
|
||
The basic problem here for the page is that we should draw an svg element as if it has no mask specified if the specified mask is display: none. (For html elements in the same situation we should not draw the html element at all.)
The fix is to treat the return values of PaintMaskSurface (which come through nsSVGIntegrationUtils::PaintMask and nsDisplayMasksAndClipPaths::PaintMask) in WebRenderCommandBuilder::BuildWrMaskImage the same way as in CreateAndPaintMaskSurface.
Updated•5 years ago
|
Comment 25•5 years ago
|
||
Comment 28•5 years ago
|
||
bugherder |
Comment 30•5 years ago
|
||
The patch landed in nightly and beta is affected.
:tnikkel, is this bug important enough to require an uplift?
If not please set status_beta
to wontfix
.
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 31•5 years ago
|
||
We've had this bug for a while and this would also require to uplift bug 1628988 which is a little more complicated, so I think we let this ride the trains.
Updated•5 years ago
|
Updated•4 years ago
|
Comment 32•4 years ago
|
||
Reproduced the issue using firefox 76.0a1 (20200324214641) on Windows 10x64 using WebRender.
The issue is verified fixed with Firefox 77.0b3 (20200507233245) with WebRender on Windows 10x64 and macOS 10.12. The images are correctly shown when visiting the page from comment 0 and the attached test cases from comment 4 and comment 5.
Updated•4 years ago
|
Updated•4 years ago
|
Description
•