Bug 85672 – [chromium] Simplify RTL root layer adjustment

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alexandre Elias]

[chromium] Simplify RTL root layer adjustment

[Alexandre Elias]

Created attachment 140335 [details]
Patch

[Xiaomei Ji]

Comment on attachment 140335 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:3389
> +    int layerAdjustX = -view->scrollOrigin().x();

how about a vertical page with rtl writing mode that having a negative axis-y? do you need a layerAdjustY too?
(pls. refer the comments before m_scrollOrigin in ScrollableArea.h)

> LayoutTests/ChangeLog:12
> +        * compositing/rtl/rtl-rootelement-scrolled-expected.png: Added.

I do not know about compositor. Just curious the png does not have scrollbar?

[Alexandre Elias]

(In reply to comment #2)
> (From update of attachment 140335 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140335&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:3389
> > +    int layerAdjustX = -view->scrollOrigin().x();
> 
> how about a vertical page with rtl writing mode that having a negative axis-y? do you need a layerAdjustY too?
> (pls. refer the comments before m_scrollOrigin in ScrollableArea.h)

Seems reasonable, it's a bit strange to special-case the X axis.  But I don't know much about writing modes, and I can't reproduce a problem with fast/writing-mode/japanese-ruby-vertical-rl.html .  Do you know a case where the y origin can be positive on the root?

> 
> > LayoutTests/ChangeLog:12
> > +        * compositing/rtl/rtl-rootelement-scrolled-expected.png: Added.
> 
> I do not know about compositor. Just curious the png does not have scrollbar?

That's because the body { overflow: hidden; } hides it.

[WebKit Review Bot]

Comment on attachment 140335 [details]
Patch

Attachment 140335 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12627278

New failing tests:
compositing/rtl/rtl-rootelement-scrolled.html

[WebKit Review Bot]

Created attachment 140341 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexandre Elias]

Created attachment 140357 [details]
Patch

[Xiaomei Ji]

Comment on attachment 140335 [details]
Patch

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

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:3389
>>> +    int layerAdjustX = -view->scrollOrigin().x();
>> 
>> how about a vertical page with rtl writing mode that having a negative axis-y? do you need a layerAdjustY too?
>> (pls. refer the comments before m_scrollOrigin in ScrollableArea.h)
> 
> Seems reasonable, it's a bit strange to special-case the X axis.  But I don't know much about writing modes, and I can't reproduce a problem with fast/writing-mode/japanese-ruby-vertical-rl.html .  Do you know a case where the y origin can be positive on the root?

try fast/dom/scroll-reveal-top-overflow.html, vertical-scrollbar-in-rtl.html in which both vertical writing mode and rtl are set.

[Alexandre Elias]

Another question for you, Xiaomei: currently, absolute-positioned elements' CSS coordinates are relative to the scrollOrigin.  So if we want an absolute-positioned element to be near to the top-left of the document, we may have to provide negative CSS coordinates.  Is this correct according to web standards?

I'm asking because seems that a few months ago (when Enne initially wrote this adjustment), it was different -- I don't have an old version to compare right now, but it seems that the CSS coordinates used to be relative to the top-left of the page even in RTL style.  I was confused about this while trying to write a layout test.

[Xiaomei Ji]

It says the following in http://www.w3.org/TR/CSS21/visuren.html#propdef-position

absolute
The box's position (and possibly size) is specified with the 'top', 'right', 'bottom', and 'left' properties. These properties specify offsets with respect to the box's containing block. Absolutely positioned boxes are taken out of the normal flow. This means they have no impact on the layout of later siblings. Also, though absolutely positioned boxes have margins, they do not collapse with any other margins.

I tried in Firefox, seems absolute-positioned elements' CSS coordinates are relative to the scrollOrigin, not  relative to the top-left of the page in RTL style.

But I am not familiar with CSS, you'd better check with experts.

[Adrienne Walker]

(In reply to comment #8)
> Another question for you, Xiaomei: currently, absolute-positioned elements' CSS coordinates are relative to the scrollOrigin.  So if we want an absolute-positioned element to be near to the top-left of the document, we may have to provide negative CSS coordinates.  Is this correct according to web standards?
> 
> I'm asking because seems that a few months ago (when Enne initially wrote this adjustment), it was different -- I don't have an old version to compare right now, but it seems that the CSS coordinates used to be relative to the top-left of the page even in RTL style.  I was confused about this while trying to write a layout test.

Looking at this, it doesn't look like anything has changed.  Absolute positioning is relative to the initial containing block and not the document itself.  If I look at rtl-absolute-overflow-scrolled, the test moves the absolute positioned elements to a negative absolute left so that it is 50px from the left side of the document and is therefore at a negative position.

Fixed position is a different story, and it looks like we have some invalidation bugs there, which I've filed as bug 85821.

Rather than add a new test, I think I'd rather modify the existing tests.  I see why they're not failing: the red indicator is just not getting drawn at all because the root layer is shifted over by the offset and so the red indicator is drawn left of the bounds of the layer.  Oops.  I've filed bug 85820 to handle this.

[Alexandre Elias]

Sounds good.  I'll remove my new test from this patch.  (I can't add one for the page-scale problem that I'm fixing because it only occurs when the page is initialized at fixed-layout, which can't be done in a layout test.)

[Adrienne Walker]

(In reply to comment #11)
> Sounds good.  I'll remove my new test from this patch.  (I can't add one for the page-scale problem that I'm fixing because it only occurs when the page is initialized at fixed-layout, which can't be done in a layout test.)

Why can't you do it in a layout test? Is this something a virtual layout test directory could solve (e.g. bug 82263)?

[Eric Seidel (no email)]

I'm a little confused why this code needs to know about RTL.

[Alexandre Elias]

It basically doesn't, but it does need to know about scrollOrigin which is affected by RTL.  This is because the compositor is unable to handle negative coordinates (which happens when scrollOrigin is positive).

But yes, I agree that there should be no explicit checking of RTL style here, and that's why patch removes it and makes the adjustment more generic.

[Eric Seidel (no email)]

Comment on attachment 140357 [details]
Patch

OK.

[Adrienne Walker]

Comment on attachment 140357 [details]
Patch

aelias said he was going to remove the test from this patch, which is redundant.  I was waiting on him.

[Alexandre Elias]

OK, one sec.

[Alexandre Elias]

Created attachment 143119 [details]
Patch

[Adrienne Walker]

Comment on attachment 143119 [details]
Patch

R=me.  Thanks.  :)

[WebKit Review Bot]

Comment on attachment 143119 [details]
Patch

Rejecting attachment 143119 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
th fuzz 3.
patching file Source/WebKit/chromium/src/WebViewImpl.cpp
Hunk #1 succeeded at 3304 (offset 180 lines).
Hunk #2 FAILED at 3403.
1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit/chromium/src/WebViewImpl.cpp.rej
patching file Source/WebKit/chromium/src/WebViewImpl.h
Hunk #1 succeeded at 495 (offset 28 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adrienne W..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12731852

[Alexandre Elias]

Created attachment 143190 [details]
Patch

[Alexandre Elias]

Resolved simple conflict.

[WebKit Review Bot]

Comment on attachment 143190 [details]
Patch

Clearing flags on attachment: 143190

Committed r117915: <http://trac.webkit.org/changeset/117915>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.