Bug 155531 – [RTL Scrollbars] Hit testing is off by the width of the vertical scrollbar

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

[Myles C. Maxfield]

[RTL Scrollbars] Hit testing is off by the width of the vertical scrollbar

[Myles C. Maxfield]

Created attachment 274171 [details]
WIP

[Myles C. Maxfield]

Created attachment 274173 [details]
Patch

[Build Bot]

Comment on attachment 274173 [details]
Patch

Attachment 274173 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/987135

New failing tests:
fast/scrolling/rtl-scrollbars-position-fixed.html
fast/scrolling/rtl-scrollbars-overflow-position-absolute.html
fast/scrolling/rtl-scrollbars-text-selection.html
fast/scrolling/rtl-scrollbars-position-absolute.html
fast/scrolling/rtl-scrollbars-text-selection-scrolled.html
fast/scrolling/rtl-scrollbars-overflow-text-selection-scrolled.html
fast/scrolling/rtl-scrollbars-overflow-elementFromPoint.html

[Build Bot]

Created attachment 274174 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 274173 [details]
Patch

Attachment 274173 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/987143

New failing tests:
fast/scrolling/rtl-scrollbars-position-fixed.html
fast/scrolling/rtl-scrollbars-overflow-position-absolute.html
fast/scrolling/rtl-scrollbars-text-selection.html
fast/scrolling/rtl-scrollbars-position-absolute.html
fast/scrolling/rtl-scrollbars-text-selection-scrolled.html
fast/scrolling/rtl-scrollbars-overflow-text-selection-scrolled.html
fast/scrolling/rtl-scrollbars-overflow-elementFromPoint.html

[Build Bot]

Created attachment 274175 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 274173 [details]
Patch

Attachment 274173 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/987144

New failing tests:
fast/scrolling/rtl-scrollbars-overflow-position-absolute.html
fast/scrolling/rtl-scrollbars-overflow-text-selection-scrolled.html
fast/scrolling/rtl-scrollbars-overflow-elementFromPoint.html

[Build Bot]

Created attachment 274176 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 274173 [details]
Patch

Attachment 274173 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/987158

New failing tests:
fast/scrolling/rtl-scrollbars-position-fixed.html
fast/scrolling/rtl-scrollbars-overflow-position-absolute.html
fast/scrolling/rtl-scrollbars-text-selection.html
fast/scrolling/rtl-scrollbars-position-absolute.html
fast/scrolling/rtl-scrollbars-text-selection-scrolled.html
fast/scrolling/rtl-scrollbars-overflow-text-selection-scrolled.html
fast/scrolling/rtl-scrollbars-overflow-elementFromPoint.html

[Build Bot]

Created attachment 274177 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Myles C. Maxfield]

ScrollView::documentScrollPositionRelativeToViewOrigin() may need to be updated. The relevant stack is:

#0	0x0000000114cf4f50 in WebCore::ScrollView::documentScrollPositionRelativeToViewOrigin() const at /Users/litherum/src/WebKit/Source/WebCore/platform/ScrollView.cpp:407
#1	0x0000000114cf6f11 in WebCore::ScrollView::viewToContents(WebCore::IntPoint const&) const at /Users/litherum/src/WebKit/Source/WebCore/platform/ScrollView.cpp:852
#2	0x0000000114cf7597 in WebCore::ScrollView::windowToContents(WebCore::IntPoint const&) const at /Users/litherum/src/WebKit/Source/WebCore/platform/ScrollView.cpp:939
#3	0x000000011360288b in WebCore::documentPointForWindowPoint(WebCore::Frame&, WebCore::IntPoint const&) at /Users/litherum/src/WebKit/Source/WebCore/page/EventHandler.cpp:1582
#4	0x0000000113603403 in WebCore::EventHandler::prepareMouseEvent(WebCore::HitTestRequest const&, WebCore::PlatformMouseEvent const&) at /Users/litherum/src/WebKit/Source/WebCore/page/EventHandler.cpp:2327

[Myles C. Maxfield]

Created attachment 274178 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 274178 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Tests: fast/scrolling/rtl-scrollbars-elementFromPoint.html

I should add a test which uses elementFromPoint but doesn't use position: absolute.

[Myles C. Maxfield]

Comment on attachment 274178 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        Tests: fast/scrolling/rtl-scrollbars-elementFromPoint.html
> 
> I should add a test which uses elementFromPoint but doesn't use position: absolute.

iframes need to be tested too, as well as testing the combination of scrolling, main document, and overflow: scroll

[Myles C. Maxfield]

Created attachment 274229 [details]
Needs more tests

[Myles C. Maxfield]

Created attachment 274256 [details]
Patch

[Myles C. Maxfield]

Created attachment 274257 [details]
Patch

[Darin Adler]

Comment on attachment 274257 [details]
Patch

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

If the tests are failing on all platforms, then the failure expectations belong in LayoutTests/TestExpectations, not repeated for each platform. I’m concerned that our TestExpectations files are getting out of hand and this repetition seems like it could be a problem.

> Source/WebCore/ChangeLog:9
> +        This patch updates WebCore::ScrollView::viewToContents() and
> +        WebCore::ScrollView::contentsToView().

I don’t understand this. It’s not literally true, so maybe it is intended to highlight the commonly used functions that will now do a better job due to the function actually being patched here? Or maybe it’s just a stale comment?

> Source/WebCore/platform/ScrollView.cpp:409
> -    return scrollPosition() - IntSize(0, headerHeight() + topContentInset(TopContentInsetType::WebCoreOrPlatformContentInset));
> +    return scrollPosition() - IntSize(
> +        ScrollableArea::systemLanguageIsRTL() && m_verticalScrollbar ? m_verticalScrollbar->occupiedWidth() : 0,
> +        headerHeight() + topContentInset(TopContentInsetType::WebCoreOrPlatformContentInset));

Something that need not block this patch, but is worth considering:

Why is the ScrollableArea prefix needed to call the systemLanguageIsRTL member function? ScrollView derives from ScrollableArea, so I would think we could omit the prefix.

Generally I think the scroll view should be asking itself whether the scroll view’s scrollbar should be on the left or right, rather than directly calling a function to ask about the system language. Of course, the function setting the policy could then turn around and call something like systemLanguageIsRTL, but it’s a bit too literal to have each call site checking that directly. There is probably very little code that has reason to depend on the fact that all scrollable areas change behavior with a single switch, which I think would be a good thing.

I think both of these comments above apply to most of the existing call sites of the ScrollableArea::systemLanguageIsRTL function. Most are inside the ScrollView class, and so they should be asking what side the scrollbar is on for this particular scroll view, not asking about a system setting directly. And outside ScrollView, RenderLayerCompositor::positionForClipLayer is asking a question of the FrameView, which is a ScrollView, so I think it should be calling an appropriate function in ScrollableArea, ScrollView, or FrameView that knows whether to return 0 or not, rather than checking ScrollableArea::systemLanguageIsRTL itself. The only case I am unclear on is RenderStyle::shouldPlaceBlockDirectionScrollbarOnLeft; at the moment am not sure how that function should change, and it seems fine to leave it as is for now.

> LayoutTests/platform/mac/TestExpectations:1347
> +[ Yosemite ElCapitan ] fast/scrolling/rtl-scrollbars-overflow-position-absolute.html [ ImageOnlyFailure ]

What is the difference between Failure and ImageOnlyFailure for a reference test? Seems like they would be the same thing. I don’t even know why we allow the concept of ImageOnlyFailure for those.

[Myles C. Maxfield]

Comment on attachment 274257 [details]
Patch

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

>> LayoutTests/platform/mac/TestExpectations:1347
>> +[ Yosemite ElCapitan ] fast/scrolling/rtl-scrollbars-overflow-position-absolute.html [ ImageOnlyFailure ]
> 
> What is the difference between Failure and ImageOnlyFailure for a reference test? Seems like they would be the same thing. I don’t even know why we allow the concept of ImageOnlyFailure for those.

"Failure" won't catch a reference test. Instead, it will only catch a text test. Vice-versa for ImageOnlyFailure and reference tests.

I'm not sure, but my guess is that we distinguish the two because you could have two expected results, an -expected.txt and an -expected.html, and one might pass while the other one fails.

[Myles C. Maxfield]

Committed r198369: <http://trac.webkit.org/changeset/198369>