yisibl Reporter
	Description • 7 years ago

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.

	u459114 Assignee
	Comment 3 • 7 years ago

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

	u459114 Assignee
	Comment 4 • 7 years ago

we probably should skip cache if there is any outflow element under the mask.

	u459114 Assignee
	Comment 12 • 7 years ago

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.

	u459114 Assignee
	Comment 13 • 7 years ago

A test page to verify the patch.

	u459114 Assignee
	Comment 14 • 7 years ago

I need to figure out when we have a out-of-flow element inside mask, why mask layer does not scroll with scroll frame.

	Markus Stange [:mstange]
	Comment 15 • 7 years ago

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.

	Markus Stange [:mstange]
	Comment 16 • 7 years ago
mozreview-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

::: 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.

	u459114 Assignee
	Comment 17 • 7 years ago
mozreview-review-reply

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.

	u459114 Assignee
	Comment 18 • 7 years ago

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

	Pulsebot
	Comment 20 • 7 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 21 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b761b30ea910

	Thomas Wisniewski
	Comment 22 • 7 years ago

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.

	u459114 Assignee
	Comment 23 • 7 years ago

(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

	u459114 Assignee
	Comment 24 • 7 years ago

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.

	Gerry Chang [:gchang]
	Comment 25 • 7 years ago

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-.