[OS X] [RTL Scrollbars] Overlay RTL scrollbars animate in from the wrong side
Created attachment 275077 [details] Patch
Created attachment 275078 [details] Patch
Comment on attachment 275078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275078&action=review > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1307 > + m_scrollableArea.setScrollbarDirection(newVerticalPainter); Can we rename the "painter"s please? Very confusing to pass a "painter" to something that apparently takes a scrollbar. > Source/WebCore/platform/mac/ScrollableAreaMac.mm:57 > +void ScrollableArea::setScrollbarDirection(NSScrollerImp *scroller) const Not sure I like the term "direction" here. I would prefer setScrollbarLayoutDirection().
Comment on attachment 275078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275078&action=review >> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1307 >> + m_scrollableArea.setScrollbarDirection(newVerticalPainter); > > Can we rename the "painter"s please? Very confusing to pass a "painter" to something that apparently takes a scrollbar. Yeah, good idea. However, doing this rename is pretty big, so I'll do it in a follow-up patch.
Committed r198784: <http://trac.webkit.org/changeset/198784>
Rolled out in https://bugs.webkit.org/show_bug.cgi?id=155999
Created attachment 275166 [details] Needs to disable Settings::setMockScrollbarsEnabled
Created attachment 275178 [details] Patch
Comment on attachment 275178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275178&action=review > Source/WebCore/platform/mac/ScrollbarThemeMac.h:69 > + bool scrollbarPainterIsRTL(Scrollbar&); Didn't we decide to get rid of the "painter" terminology? > Source/WebCore/rendering/RenderTreeAsText.cpp:633 > + ts << " RTL"; I think this is a bit confusing. To me this would imply "scrollbars on the left", rather than "scrollbar has RTL layout direction". > LayoutTests/fast/scrolling/rtl-scrollbars-animation-property.html:1 > +<!DOCTYPE html><!-- webkit-test-runner [ dontMockScrollbars=true ] --> Why the [ dontMockScrollbars=true ] as well as setMockScrollbarsEnabled(false) ?
Comment on attachment 275178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275178&action=review Despite the concerns from both Simon and me, I am going to mark this review+ but you should probably fix the things he asked about. > Source/WebCore/platform/mac/ScrollbarThemeMac.h:67 > + void didCreateScrollerImp(Scrollbar&); This should be a static member function. >> Source/WebCore/platform/mac/ScrollbarThemeMac.h:69 >> + bool scrollbarPainterIsRTL(Scrollbar&); > > Didn't we decide to get rid of the "painter" terminology? How about just calling this isLayoutDirectionRTL? This should be a static member function. Annoying to have this here just for logging purposes. Would be better to paragraph this with the other ScrollerImp function just above, I think. Omit the blank line.
Comment on attachment 275178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275178&action=review >> Source/WebCore/platform/mac/ScrollbarThemeMac.h:67 >> + void didCreateScrollerImp(Scrollbar&); > > This should be a static member function. I don't think it can be static because the implementation calls painterForScrollbar(). >>> Source/WebCore/platform/mac/ScrollbarThemeMac.h:69 >>> + bool scrollbarPainterIsRTL(Scrollbar&); >> >> Didn't we decide to get rid of the "painter" terminology? > > How about just calling this isLayoutDirectionRTL? > > This should be a static member function. > > Annoying to have this here just for logging purposes. > > Would be better to paragraph this with the other ScrollerImp function just above, I think. Omit the blank line. Same comment about painterForScrollbar(). >> LayoutTests/fast/scrolling/rtl-scrollbars-animation-property.html:1 >> +<!DOCTYPE html><!-- webkit-test-runner [ dontMockScrollbars=true ] --> > > Why the [ dontMockScrollbars=true ] as well as setMockScrollbarsEnabled(false) ? Whoops, this is left over from an older version of this patch.
Created attachment 275201 [details] Patch for committing
Comment on attachment 275201 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=275201&action=review > Source/WebCore/platform/mac/ScrollbarThemeMac.h:67 > + static void didCreateScrollerImp(Scrollbar&); Oops, you said this wouldn’t work!
(In reply to comment #13) > Comment on attachment 275201 [details] > Patch for committing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275201&action=review > > > Source/WebCore/platform/mac/ScrollbarThemeMac.h:67 > > + static void didCreateScrollerImp(Scrollbar&); > > Oops, you said this wouldn’t work! :( Whoops. I only compiled this on El Capitan.... where it doesn't call the non-static function. (This is why I posted the patch so I can do more investigation today)
Committed r198859: <http://trac.webkit.org/changeset/198859>
(In reply to comment #15) > Committed r198859: <http://trac.webkit.org/changeset/198859> Follow-up gardening fixes: EFL: r198944 <http://trac.webkit.org/changeset/198944> GTK/iOS-Simulator/Windows: r198982 <http://trac.webkit.org/changeset/198982>
(In reply to comment #16) > (In reply to comment #15) > > Committed r198859: <http://trac.webkit.org/changeset/198859> > > Follow-up gardening fixes: > > EFL: r198944 <http://trac.webkit.org/changeset/198944> > GTK/iOS-Simulator/Windows: r198982 <http://trac.webkit.org/changeset/198982> See: <http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&revision=198981&tests=fast%2Fscrolling%2Frtl-scrollbars-animation-property.html>
<rdar://problem/25118321>
Comment on attachment 275201 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=275201&action=review > LayoutTests/platform/mac/fast/scrolling/rtl-scrollbars-animation-property-expected.txt:8 > +layer at (0,0) size 785x2000 > + RenderView at (0,0) size 785x600 > +layer at (0,0) size 785x216 > + RenderBlock {HTML} at (0,0) size 785x216 > + RenderBody {BODY} at (8,8) size 769x200 > +layer at (8,8) size 200x200 clip at (8,8) size 185x185 scrollHeight 2000 > + RenderBlock (relative positioned) {DIV} at (0,0) size 200x200 > +layer at (8,8) size 1x2000 backgroundClip at (8,8) size 185x185 clip at (8,8) size 185x185 Hi Myles. Post-land question: Could you help me to understand how this test result ensures RTL animation correctness?
(In reply to comment #19) > Comment on attachment 275201 [details] > Patch for committing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275201&action=review > > > LayoutTests/platform/mac/fast/scrolling/rtl-scrollbars-animation-property-expected.txt:8 > > +layer at (0,0) size 785x2000 > > + RenderView at (0,0) size 785x600 > > +layer at (0,0) size 785x216 > > + RenderBlock {HTML} at (0,0) size 785x216 > > + RenderBody {BODY} at (8,8) size 769x200 > > +layer at (8,8) size 200x200 clip at (8,8) size 185x185 scrollHeight 2000 > > + RenderBlock (relative positioned) {DIV} at (0,0) size 200x200 > > +layer at (8,8) size 1x2000 backgroundClip at (8,8) size 185x185 clip at (8,8) size 185x185 > > Hi Myles. Post-land question: > > Could you help me to understand how this test result ensures RTL animation > correctness? RTL scrollbars are only supported on certain operating systems, and this file is not used on those operating systems. (On those operating systems, the -expected.txt has a line that says "layer at (8,8) size 200x200 clip at (23,8) size 185x185 scrollHeight 2000 scrollbarHasRTLLayoutDirection") On OS X, we don't have any infrastructure to actually test the animation itself (because the animation is implemented by another framework on OS X). Instead, this test just makes sure that we set the appropriate state to tell the other framework to do the correct animation.