Bug 88762 - Invalidation rects are not scaled for position:fixed elements
Summary: Invalidation rects are not scaled for position:fixed elements
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-06-11 00:24 PDT by Adam Barth
Modified: 2012-10-17 13:13 PDT (History)
10 users (show)

See Also:


Attachments
Patch (needs tests) (1.85 KB, patch)
2012-06-11 00:25 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Attempt at a test (943 bytes, patch)
2012-06-11 18:14 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
updated test (28 bytes, patch)
2012-06-21 13:32 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
updated test (1.12 KB, patch)
2012-06-21 13:33 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
updated test (1.02 KB, patch)
2012-06-21 13:37 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Working test (1.54 KB, patch)
2012-06-22 08:03 PDT, Sami Kyöstilä
no flags Details | Formatted Diff | Diff
test with expected results (3.22 KB, patch)
2012-06-24 02:21 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-06-11 00:24:37 PDT
Invalidation rects are not scaled for position:fixed elements
Comment 1 Adam Barth 2012-06-11 00:25:20 PDT
Created attachment 146797 [details]
Patch (needs tests)
Comment 2 Adam Barth 2012-06-11 00:27:12 PDT
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!
Comment 3 Antonio Gomes 2012-06-11 06:57:08 PDT
is this possible to come with a regression test?
Comment 4 Sami Kyostila 2012-06-11 07:10:49 PDT
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.
Comment 5 Adam Barth 2012-06-11 10:28:04 PDT
Thanks Sami.  I'll give that a try.
Comment 6 Adam Barth 2012-06-11 18:14:41 PDT
Created attachment 146985 [details]
Attempt at a test
Comment 7 Adam Barth 2012-06-11 18:15:21 PDT
This test seems to pass without the patch.  Any ideas what I'm doing wrong?  Thanks!
Comment 8 Adam Barth 2012-06-11 18:21:16 PDT
Note: This test does note appear to execute the lines changed by this patch.
Comment 9 Sami Kyöstilä 2012-06-19 08:23:47 PDT
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.
Comment 10 Adam Barth 2012-06-21 13:32:55 PDT
Created attachment 148873 [details]
updated test
Comment 11 Adam Barth 2012-06-21 13:33:26 PDT
Created attachment 148874 [details]
updated test
Comment 12 Adam Barth 2012-06-21 13:35:26 PDT
> 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.
Comment 13 Adam Barth 2012-06-21 13:37:32 PDT
Created attachment 148876 [details]
updated test
Comment 14 Adam Barth 2012-06-21 13:38:18 PDT
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?
Comment 15 Sami Kyöstilä 2012-06-22 08:02:52 PDT
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 :)
Comment 16 Sami Kyöstilä 2012-06-22 08:03:42 PDT
Created attachment 149031 [details]
Working test
Comment 17 Adam Barth 2012-06-22 10:15:35 PDT
Ah, thanks for the test!
Comment 18 Adam Barth 2012-06-24 02:21:49 PDT
Created attachment 149190 [details]
test with expected results
Comment 19 Adam Barth 2012-06-24 02:24:37 PDT
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.
Comment 20 Sami Kyöstilä 2012-07-02 02:51:38 PDT
Sure, I can try to come up with fix.
Comment 21 Adam Barth 2012-10-17 13:13:12 PDT
I've posted a change to remove this diff from the downstream branch.