[RTL Scrollbars] Hit testing is off by the width of the vertical scrollbar
Created attachment 274171 [details] WIP
Created attachment 274173 [details] Patch
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
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
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
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
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
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
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
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
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
Created attachment 274178 [details] Patch
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.
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
Created attachment 274229 [details] Needs more tests
Created attachment 274256 [details] Patch
Created attachment 274257 [details] Patch
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.
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.
Committed r198369: <http://trac.webkit.org/changeset/198369>