Closed Bug 1350663 Opened 6 years ago Closed 6 years ago

[css-mask] mask-image failure at position: fixed

Categories

(Core :: Layout, defect)

55 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: percyley, Assigned: u459114)

References

Details

Attachments

(4 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.110 Safari/537.36

Steps to reproduce:

Open http://razvancaliman.com/cssmasking/ and scroll down page


Actual results:

 "Trekking" text is not hidden.


Expected results:

Chrome will hide text.
Attached image firefox.png
Attached image chrome.png
Assignee: nobody → cku
Status: UNCONFIRMED → NEW
Ever confirmed: true
See Also: → 1363855
Patch in bug 1362000 fix invalidation check for svg-mask.
This bug is another case - invalid test for image mask is not sufficien as well.
By removing the if statment at [1], we won't see this bug again.

Still working on it.

[1] https://hg.mozilla.org/mozilla-central/file/a2832968581c/layout/painting/FrameLayerBuilder.cpp#l3884
we probably should skip cache if there is any outflow element under the mask.
Attachment #8867523 - Flags: review?(mstange)
Attachment #8867523 - Flags: review?(mstange)
Attachment #8867523 - Flags: review?(mstange)
When e10 is turning off, this patch can fix this bug totally. Unforunately, when e10s is on, this patch fix only part of it.
When e10s turns on, we do not have a change to repatin mask while scrolling, so the content of mask keep wrong until scroll finished.

But I still think we should land this patch first, since it can fix a reftest failure in bug 1358055.
Attached file scroll-this-page.html
A test page to verify the patch.
I need to figure out when we have a out-of-flow element inside mask, why mask layer does not scroll with scroll frame.
Hi CJ, sorry I'm taking so long to review this. I was aware of a problem with APZ and mask layers that wrap fixed position frames, and since this patch didn't seem to touch anything related to APZ, I wanted to test whether it actually helps when APZ is used. But I hadn't got around to doing that.

So now that we know that it's not going to fix the problem with APZ, I'm less confused about what the patch is doing.
See Also: → 1300864
Comment on attachment 8867523 [details]
Bug 1350663 - Repaint mask layer when the offset between mask region and mask layer changed.

https://reviewboard.mozilla.org/r/139076/#review147046

::: commit-message-68d7a:46
(Diff revision 7)
> +   rejected.
> +
> +It's difficult to create a reftest for this bug as well. With scrollTo, we may
> +mimic an environment of this error, but since reftest harness will trigger
> +whole viewport repaint while taking a snapshot, we actually can not repro this
> +issue on it.

I think it should be quite possible to write a test for this. See 1155828-1.html or 1025914-1.html for examples.
Attachment #8867523 - Flags: review?(mstange) → review+
Comment on attachment 8867523 [details]
Bug 1350663 - Repaint mask layer when the offset between mask region and mask layer changed.

https://reviewboard.mozilla.org/r/139076/#review147046

> I think it should be quite possible to write a test for this. See 1155828-1.html or 1025914-1.html for examples.

You are right, write a reftest for this is possible, but make it failed is not easy.
When we take a snapshot in reftest harness, we will "repaint". When we repaint, make layer repaint; if mask layer repaint, test case pass.
Keep chatting log on IRC:
mstange> CJKu: the position:fixed inside mask layer with APZ problem is a bit tricky, I think
CJKu: I think we should fix it in a different bug
I was trying to fix it a few weeks ago but ran into some problems, and then I mostly forgot what I had done
I will try to find my patch and explain the problems I ran into
<CJKu> mstange: I notice, even I turn off mask layer, use indirect paint(nsDispalyMask::PaintAsLayer), the result is still wrong  
<mstange> CJKu: oh, that's surprising
or no, it's not actually that surprising
CJKu: so with APZ we use "paint skipping" to avoid painting at all if all we did was scroll
CJKu: http://searchfox.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2827
CJKu: and position:fixed inside masks is something that APZ can't handle, so paint skipping is technically wrong for this case
CJKu: we have a very similar problem with filters, let me find the bug about that
mstange> CJKu: https://bugzilla.mozilla.org/show_bug.cgi?id=1300864
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b761b30ea910
Repaint mask layer when the offset between mask region and mask layer changed. r=mstange
https://hg.mozilla.org/mozilla-central/rev/b761b30ea910
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1368234
Is there any chance of this fix being uplifted to 54? It seems to fix an issue on at least one site using CSS clip-paths to achieve a fancy scrolling effect, as can be seen in the webcompat issue: https://webcompat.com/issues/7009

That is, on my Macbook with Intel Iris Pro, there are black/invalidated regions while/after scrolling, in builds before the mozregression range including this bug: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bce03a8eac301bcd9408b22333b1a67c3eaed057&tochange=1c1bf54915bbf8cda82dbf7985e87b1c0ff30e4c

Mozregression also tells me that the issue started with the fix in bug 1325550.

Also note that this fix doesn't seem to help completely on the affected page. There are still black/invalid regions while the page is scrolling until it stops, at which point they finally vanish.
Flags: needinfo?(cku)
(In reply to Thomas Wisniewski from comment #22)
> Is there any chance of this fix being uplifted to 54? It seems to fix an
> issue on at least one site using CSS clip-paths to achieve a fancy scrolling
> effect, as can be seen in the webcompat issue:
> https://webcompat.com/issues/7009
> 
> That is, on my Macbook with Intel Iris Pro, there are black/invalidated
> regions while/after scrolling, in builds before the mozregression range
> including this bug:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=bce03a8eac301bcd9408b22333b1a67c3eaed057&tochange=1c1b
> f54915bbf8cda82dbf7985e87b1c0ff30e4c
> 
> Mozregression also tells me that the issue started with the fix in bug
> 1325550.
> 
> Also note that this fix doesn't seem to help completely on the affected
> page. There are still black/invalid regions while the page is scrolling
> until it stops, at which point they finally vanish.

It's a little bit late for ride it on 54... but since this fix is simple. Let's ask for uplift and see what Sheriff think
Flags: needinfo?(cku)
Comment on attachment 8867523 [details]
Bug 1350663 - Repaint mask layer when the offset between mask region and mask layer changed.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1325550
[User impact if declined]: mask content can be wrong with out-of-flow masked content.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: NA
[Is the change risky?]: no
[Why is the change risky/not risky?]: very simple change and landed on central for two weeks.
[String changes made/needed]: N/A

I know this is a little bit late for uplift it to 54, but since this patch is simple and can fix a webcompact issue, I do suggest we should do it.
Attachment #8867523 - Flags: approval-mozilla-beta?
Comment on attachment 8867523 [details]
Bug 1350663 - Repaint mask layer when the offset between mask region and mask layer changed.

It's too late for 54. Beta54-.
Attachment #8867523 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.