Bug 60741 - Incorrect RenderLayer transforms on overflow RTL pages
Summary: Incorrect RenderLayer transforms on overflow RTL pages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords: InRadar
Depends on:
Blocks: 56591
  Show dependency treegraph
 
Reported: 2011-05-12 18:33 PDT by Adrienne Walker
Modified: 2011-06-24 12:16 PDT (History)
6 users (show)

See Also:


Attachments
Test case: compositing, fixed position layer (343 bytes, text/html)
2011-05-12 18:34 PDT, Adrienne Walker
no flags Details
Test case: software, fixed position layer (298 bytes, text/html)
2011-05-12 18:34 PDT, Adrienne Walker
no flags Details
Test case: compositing, absolute position layer (346 bytes, text/html)
2011-05-12 18:35 PDT, Adrienne Walker
no flags Details
Test case: software, fixed position layer (301 bytes, text/html)
2011-05-12 18:35 PDT, Adrienne Walker
no flags Details
Patch (50.82 KB, patch)
2011-06-23 14:23 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (51.71 KB, patch)
2011-06-23 15:17 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (52.11 KB, patch)
2011-06-24 11:13 PDT, Adrienne Walker
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-05-12 18:33:24 PDT
On pages with dir=rtl, compositing, and a horizontal scrollbar, composited layers get incorrect transforms and bounds.  Without any horizontal overflow, the composited case appears identical to the software case.

At the very least, http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderLayerCompositor.cpp#L444 assumes that the content origin is at 0, so when boundingBoxRect.x() is negative, that calculation ends up with a very large bound.

I will attach some test cases to demonstrate this issue by resizing the window.  Compare the composited cases (with and without a horizontal scrollbar) to the software cases.
Comment 1 Adrienne Walker 2011-05-12 18:34:29 PDT
Created attachment 93380 [details]
Test case: compositing, fixed position layer
Comment 2 Adrienne Walker 2011-05-12 18:34:44 PDT
Created attachment 93381 [details]
Test case: software, fixed position layer
Comment 3 Adrienne Walker 2011-05-12 18:35:01 PDT
Created attachment 93382 [details]
Test case: compositing, absolute position layer
Comment 4 Adrienne Walker 2011-05-12 18:35:42 PDT
Created attachment 93383 [details]
Test case: software, fixed position layer
Comment 5 mitz 2011-05-20 15:16:33 PDT
<rdar://problem/9479308>
Comment 6 Adrienne Walker 2011-06-23 14:23:33 PDT
Created attachment 98402 [details]
Patch
Comment 7 Adrienne Walker 2011-06-23 14:25:18 PDT
I will also mention that I have an in-progress patch that reenables compositing for rtl pages in Chromium.  With that patch in place, these tests pass in Chromium.  I just wanted to fix this underlying general WebKit bug first before committing that code.
Comment 8 Simon Fraser (smfr) 2011-06-23 14:25:31 PDT
Comment on attachment 98402 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98402&action=review

> LayoutTests/compositing/rtl/rtl-absolute-overflow-scrolled.html:54
> +    <!-- green square should appear at 50,50 from the top left -->
> +    <div id="layer"></div>

It's better if red shows on failure. Can you put a red box under the green one that would be exposed on failure in all the tests?
Comment 9 James Robinson 2011-06-23 14:26:43 PDT
Comment on attachment 98402 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98402&action=review

> Source/WebCore/ChangeLog:9
> +        Fix positioning of the root graphics layer for RTL pages.  The fact
> +        the left side of the page is negative for pages with overflow is not

I think you accidentally a word ("The fact the left side..")
Comment 10 Adrienne Walker 2011-06-23 15:17:38 PDT
Created attachment 98418 [details]
Patch
Comment 11 Adrienne Walker 2011-06-23 15:19:04 PDT
(In reply to comment #10)
> Created an attachment (id=98418) [details]
> Patch

Grammar fixed.  The tests now have non-composited red indicator elements positioned identically underneath the composited green layers.
Comment 12 Adrienne Walker 2011-06-23 16:14:09 PDT
Committed r89632: <http://trac.webkit.org/changeset/89632>
Comment 13 Simon Fraser (smfr) 2011-06-23 16:33:02 PDT
Can you list some real-world sites that are affected by this change?
Comment 14 Adrienne Walker 2011-06-23 17:04:45 PDT
(In reply to comment #13)
> Can you list some real-world sites that are affected by this change?

This could very plausibly trigger on Gmail in Hebrew.  If there's an embedded video in a message, the Gmail fixed position "archive spam delete etc" header could overlap it, become composited, and would then draw entirely incorrectly.
Comment 15 Adrienne Walker 2011-06-23 17:22:25 PDT
Committed r89640: <http://trac.webkit.org/changeset/89640>
Comment 16 Adrienne Walker 2011-06-23 17:59:56 PDT
Reverted r89632 for reason:

Breaks ancestor-overflow-change unexpectedly

Committed r89645: <http://trac.webkit.org/changeset/89645>
Comment 17 Adrienne Walker 2011-06-23 18:03:12 PDT
(In reply to comment #16)
> Reverted r89632 for reason:
> 
> Breaks ancestor-overflow-change unexpectedly
> 
> Committed r89645: <http://trac.webkit.org/changeset/89645>

I thought I had tested this on Safari, but I missed it and it's skipped on Chromium Linux.

I will fix this issue and reupload.  Sorry for the hassle.  :)
Comment 18 Adrienne Walker 2011-06-24 11:13:27 PDT
Created attachment 98514 [details]
Patch
Comment 19 Adrienne Walker 2011-06-24 11:18:46 PDT
(In reply to comment #18)
> Created an attachment (id=98514) [details]
> Patch

Now with fewer test breakages and some additional skipped tests on Chromium Mac.  This ran cleanly on Linux Chromium-GPU and on Snow Leopard Safari.

Root graphics layers need to be adjusted so that their position is zero and their offset is the left of the document.  The previous patch was using the local compositing bounds for the adjustment and not the document rect, which was not quite the right value in all cases.
Comment 20 Adrienne Walker 2011-06-24 11:35:23 PDT
Committed r89687: <http://trac.webkit.org/changeset/89687>
Comment 21 Adrienne Walker 2011-06-24 12:16:59 PDT
Committed r89692: <http://trac.webkit.org/changeset/89692>