Bug 112088 – Correct coordinated scrolling for RTL iframe and overflow:scroll

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

[Tien-Ren Chen]

Correct impl-side scrolling for RTL iframe and overflow:scroll

[Tien-Ren Chen]

Created attachment 192624 [details]
Patch

[WebKit Review Bot]

Attachment 192624 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/compositing/rtl/rtl-overflow-scrolling-expected.png', u'LayoutTests/compositing/rtl/rtl-overflow-scrolling-expected.txt', u'LayoutTests/compositing/rtl/rtl-overflow-scrolling.html', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/tests/ScrollingCoordinatorChromiumTest.cpp', u'Source/WebKit/chromium/tests/data/rtl-iframe-inner.html', u'Source/WebKit/chromium/tests/data/rtl-iframe.html']" exit_code: 1
LayoutTests/compositing/rtl/rtl-overflow-scrolling-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 192624 [details]
Patch

Attachment 192624 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17041287

New failing tests:
compositing/rtl/rtl-overflow-scrolling.html

[Build Bot]

Comment on attachment 192624 [details]
Patch

Attachment 192624 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17201011

New failing tests:
fast/css/sticky/inline-sticky-abspos-child.html
compositing/rtl/rtl-overflow-scrolling.html

[Tien-Ren Chen]

(In reply to comment #4)
> New failing tests:
> fast/css/sticky/inline-sticky-abspos-child.html

Weird sub-pixel differences. I think even the expected output isn't correct. Ahem X is supposed to be 1em*1em solid box, but somehow it is not pixel-snapped on mac-wk2.

I can change the tests to use 24px*24px inline-block instead of Ahem, or should we just mark it as ImageOnlyFailure? Which one is a preferred solution?

> compositing/rtl/rtl-overflow-scrolling.html

Difference in scrollbar behavior. I'll update my expectation for mac.

[Tien-Ren Chen]

(In reply to comment #5)
> (In reply to comment #4)
> > New failing tests:
> > fast/css/sticky/inline-sticky-abspos-child.html
> 
> Weird sub-pixel differences. I think even the expected output isn't correct. Ahem X is supposed to be 1em*1em solid box, but somehow it is not pixel-snapped on mac-wk2.
> 
> I can change the tests to use 24px*24px inline-block instead of Ahem, or should we just mark it as ImageOnlyFailure? Which one is a preferred solution?

The baseline is 0.2em above its bottom, so the vertical range of the solid box is effectively -4.8~19.2 for 24px texts. Changing it to 25px solved the problem. There are still rounding issues due to that 0.2 is not a machine number, so the test hash doesn't match, however the error is good enough to pass image diff

[Tien-Ren Chen]

Created attachment 192828 [details]
Patch

[Sami Kyöstilä]

I'm surprised to see there isn't more fallout from changing the RenderLayer's scroll coordinate system like this.

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

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:251
> +        webLayer->setScrollPosition(IntPoint() + (scrollableArea->scrollPosition() - scrollableArea->minimumScrollPosition()));

Nit: I find toPoint() a little more concise than this but both work.

> Source/WebCore/rendering/RenderLayer.cpp:2539
> +    return IntPoint() + m_scrollOffset;

With this change this method is equivalent to scrolledContentOffset. Do we need both?

[Tien-Ren Chen]

(In reply to comment #8)
> I'm surprised to see there isn't more fallout from changing the RenderLayer's scroll coordinate system like this.

Because scrollOrigin is (0, 0) most of the times. :p

In fact the scrollPosition family functions use scrollOrigin in the wrong way. The scrollPosition got out of range all the time...

> View in context: https://bugs.webkit.org/attachment.cgi?id=192828&action=review
> 
> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:251
> > +        webLayer->setScrollPosition(IntPoint() + (scrollableArea->scrollPosition() - scrollableArea->minimumScrollPosition()));
> 
> Nit: I find toPoint() a little more concise than this but both work.

Will do.

> > Source/WebCore/rendering/RenderLayer.cpp:2539
> > +    return IntPoint() + m_scrollOffset;
> 
> With this change this method is equivalent to scrolledContentOffset. Do we need both?

Probably not. Let me investigate why we introduced the other.

[Tien-Ren Chen]

Created attachment 193695 [details]
Patch

[Tien-Ren Chen]

(In reply to comment #9)
> (In reply to comment #8)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=192828&action=review
> > 
> > > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:251
> > > +        webLayer->setScrollPosition(IntPoint() + (scrollableArea->scrollPosition() - scrollableArea->minimumScrollPosition()));
> > 
> > Nit: I find toPoint() a little more concise than this but both work.
> 
> Will do.

toPoint() has been removed. Using IntPoint() instead.

> > > Source/WebCore/rendering/RenderLayer.cpp:2539
> > > +    return IntPoint() + m_scrollOffset;
> > 
> > With this change this method is equivalent to scrolledContentOffset. Do we need both?
> 
> Probably not. Let me investigate why we introduced the other.

I'm not sure. We definitely need scrollPosition() family of functions for ScrollableArea. scrolledContentOffset() was introduced in a overflow:scroll refactoring. CCing jchaffraix@ since he knows the best.

[James Robinson]

Comment on attachment 193695 [details]
Patch

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

The changes to the meaning of the RenderLayer functions isn't obvious.  Could you explain what you are changing and why (preferably in the ChangeLogs for these functions)?

> Source/WebCore/ChangeLog:3
> +        Correct impl-side scrolling for RTL iframe and overflow:scroll

"impl-side" doesn't really have a meaning in WebKit. could you maybe change it to coordinated scrollbars?

[Tien-Ren Chen]

Created attachment 193912 [details]
Patch

[James Robinson]

Comment on attachment 193912 [details]
Patch

This is r- without an explanation of the changes to RenderLayer describing what these functions used to mean, what they mean now, and why they're changed.

[Tien-Ren Chen]

Created attachment 193945 [details]
Patch

[James Robinson]

Comment on attachment 193945 [details]
Patch

Great, that's really useful to have.  R=me

[WebKit Review Bot]

Comment on attachment 193945 [details]
Patch

Clearing flags on attachment: 193945

Committed r146399: <http://trac.webkit.org/changeset/146399>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.