Bug 88762

Created attachment 146797 [details] Patch (needs tests)

This patch exists in the chromium-android branch, but I'm somewhat out of my area here. Do you know if this patch is correct? If so, how do we write a test for it? Thanks!

is this possible to come with a regression test?

I think it should be possible to test this with window.internals.settings.setPageScaleFactor(), window.scrollBy() and changing the contents of the fixed position element.

Thanks Sami. I'll give that a try.

Created attachment 146985 [details] Attempt at a test

This test seems to pass without the patch. Any ideas what I'm doing wrong? Thanks!

Note: This test does note appear to execute the lines changed by this patch.

Looks like calling scrollTo() in an inline <script> tag is a bit too early, so FrameView skips the fast blit scroll path and we don't end up in the changed code path. Moving the scrollTo() to repaintTest() makes it work. I tried the patch both on the master and android branches, and on the former it does the right thing, but on master updateRect ends up getting scaled twice. That is, m_repaintRect for the fixed RenderBox already includes page scale. I'm not sure where the discrepancy comes from, so this needs a little more investigation. By the way, I was a bit off on how to reproduce this: you only need to have a (non-layer) fixed position element and then apply a scroll. There's no need to invalidate the fixed position element itself, because this bug is about painting the stuff behind the fixed position element.

Created attachment 148873 [details] updated test

Created attachment 148874 [details] updated test

> Looks like calling scrollTo() in an inline <script> tag is a bit too early, so FrameView skips the fast blit scroll path and we don't end up in the changed code path. Moving the scrollTo() to repaintTest() makes it work. Ok. I've updated the test to do that. However, I can't get the bug to manifest. > I tried the patch both on the master and android branches, and on the former it does the right thing, but on master updateRect ends up getting scaled twice. That is, m_repaintRect for the fixed RenderBox already includes page scale. Have you successfully replicated the bug on trunk? > I'm not sure where the discrepancy comes from, so this needs a little more investigation. > > By the way, I was a bit off on how to reproduce this: you only need to have a (non-layer) fixed position element and then apply a scroll. There's no need to invalidate the fixed position element itself, because this bug is about painting the stuff behind the fixed position element. Ok. Let me try to update the test again.

Created attachment 148876 [details] updated test

Comment on attachment 148874 [details] updated test Ok. I stil can't reproduce the issue. If you're able to reproduce the issue, would you be willing to post the test case that reproduces it?

Ok, I figured out what was causing the test to not show the problem: we need to be composited mode. As soon as that happens, the invalidation rects lose their page scale and the result is corrupted. Coincidentally the fix we have in the Android branch is wrong, because it invalidations in non-composited mode. Some more work is still needed, but at least we have a test now :)

Created attachment 149031 [details] Working test

Ah, thanks for the test!

Created attachment 149190 [details] test with expected results

Unfortunately, that means this bug is in an area of the code I'm not familiar with. Sami, do you know who would be a good person to work on a proper fix? One possibility is hclam, who made the original change to the chromium-android branch.

Sure, I can try to come up with fix.

I've posted a change to remove this diff from the downstream branch.