Bug 85672 - [chromium] Simplify RTL root layer adjustment
Summary: [chromium] Simplify RTL root layer adjustment
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandre Elias
URL:
Keywords:
Depends on: 85820
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-04 14:57 PDT by Alexandre Elias
Modified: 2012-05-21 23:43 PDT (History)
8 users (show)

See Also:


Attachments
Patch (7.29 KB, patch)
2012-05-04 15:05 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (5.84 MB, application/zip)
2012-05-04 15:42 PDT, WebKit Review Bot
no flags Details
Patch (7.15 KB, patch)
2012-05-04 16:21 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (3.70 KB, patch)
2012-05-21 15:51 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (3.48 KB, patch)
2012-05-21 22:36 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Elias 2012-05-04 14:57:47 PDT
[chromium] Simplify RTL root layer adjustment
Comment 1 Alexandre Elias 2012-05-04 15:05:58 PDT
Created attachment 140335 [details]
Patch
Comment 2 Xiaomei Ji 2012-05-04 15:24:48 PDT
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?
Comment 3 Alexandre Elias 2012-05-04 15:40:26 PDT
(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.
Comment 4 WebKit Review Bot 2012-05-04 15:42:28 PDT
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
Comment 5 WebKit Review Bot 2012-05-04 15:42:33 PDT
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
Comment 6 Alexandre Elias 2012-05-04 16:21:49 PDT
Created attachment 140357 [details]
Patch
Comment 7 Xiaomei Ji 2012-05-04 16:29:18 PDT
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.
Comment 8 Alexandre Elias 2012-05-04 16:36:48 PDT
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.
Comment 9 Xiaomei Ji 2012-05-04 17:48:55 PDT
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.
Comment 10 Adrienne Walker 2012-05-07 13:27:13 PDT
(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.
Comment 11 Alexandre Elias 2012-05-07 13:31:32 PDT
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.)
Comment 12 Adrienne Walker 2012-05-07 14:21:01 PDT
(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)?
Comment 13 Eric Seidel (no email) 2012-05-21 15:32:46 PDT
I'm a little confused why this code needs to know about RTL.
Comment 14 Alexandre Elias 2012-05-21 15:35:14 PDT
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.
Comment 15 Eric Seidel (no email) 2012-05-21 15:44:40 PDT
Comment on attachment 140357 [details]
Patch

OK.
Comment 16 Adrienne Walker 2012-05-21 15:45:50 PDT
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.
Comment 17 Alexandre Elias 2012-05-21 15:46:45 PDT
OK, one sec.
Comment 18 Alexandre Elias 2012-05-21 15:51:00 PDT
Created attachment 143119 [details]
Patch
Comment 19 Adrienne Walker 2012-05-21 15:51:54 PDT
Comment on attachment 143119 [details]
Patch

R=me.  Thanks.  :)
Comment 20 WebKit Review Bot 2012-05-21 20:47:51 PDT
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
Comment 21 Alexandre Elias 2012-05-21 22:36:20 PDT
Created attachment 143190 [details]
Patch
Comment 22 Alexandre Elias 2012-05-21 22:37:09 PDT
Resolved simple conflict.
Comment 23 WebKit Review Bot 2012-05-21 23:43:26 PDT
Comment on attachment 143190 [details]
Patch

Clearing flags on attachment: 143190

Committed r117915: <http://trac.webkit.org/changeset/117915>
Comment 24 WebKit Review Bot 2012-05-21 23:43:32 PDT
All reviewed patches have been landed.  Closing bug.