Bug 88762 – Invalidation rects are not scaled for position:fixed elements

• Status: ASSIGNED
• Component: New Bugs

[Adam Barth]

Invalidation rects are not scaled for position:fixed elements

[Adam Barth]

Created attachment 146797 [details]
Patch (needs tests)

[Adam Barth]

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!

[Antonio Gomes]

is this possible to come with a regression test?

[Sami Kyostila]

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.

[Adam Barth]

Thanks Sami.  I'll give that a try.

[Adam Barth]

Created attachment 146985 [details]
Attempt at a test

[Adam Barth]

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

[Adam Barth]

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

[Sami Kyöstilä]

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.

[Adam Barth]

Created attachment 148873 [details]
updated test

[Adam Barth]

Created attachment 148874 [details]
updated test

[Adam Barth]

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

[Adam Barth]

Created attachment 148876 [details]
updated test

[Adam Barth]

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?

[Sami Kyöstilä]

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

[Sami Kyöstilä]

Created attachment 149031 [details]
Working test

[Adam Barth]

Ah, thanks for the test!

[Adam Barth]

Created attachment 149190 [details]
test with expected results

[Adam Barth]

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.

[Sami Kyöstilä]

Sure, I can try to come up with fix.

[Adam Barth]

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