Bug 85672

Created attachment 140335 [details] Patch

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?

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

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

Created attachment 140357 [details] Patch

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.

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.

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.

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

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

(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)?

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

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 on attachment 140357 [details] Patch OK.

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.

OK, one sec.

Created attachment 143119 [details] Patch

Comment on attachment 143119 [details] Patch R=me. Thanks. :)

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

Created attachment 143190 [details] Patch

Resolved simple conflict.

Comment on attachment 143190 [details] Patch Clearing flags on attachment: 143190 Committed r117915: <http://trac.webkit.org/changeset/117915>

All reviewed patches have been landed. Closing bug.