Bug 155962

Summary: [OS X] [RTL Scrollbars] Overlay RTL scrollbars animate in from the wrong side
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: Layout and RenderingAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, darin, ddkilzer, dino, esprehn+autocc, glenn, jonlee, kondapallykalyan, simon.fraser, thorton, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 155999    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Needs to disable Settings::setMockScrollbarsEnabled
none
Patch
darin: review+
Patch for committing none

Myles C. Maxfield
Reported 2016-03-28 19:29:16 PDT
[OS X] [RTL Scrollbars] Overlay RTL scrollbars animate in from the wrong side
Attachments
Patch (8.52 KB, patch)
2016-03-28 19:35 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Patch (8.53 KB, patch)
2016-03-28 19:43 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Needs to disable Settings::setMockScrollbarsEnabled (10.30 KB, patch)
2016-03-29 19:46 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Patch (15.50 KB, patch)
2016-03-29 21:27 PDT, Myles C. Maxfield 		darin: review+
DetailsFormatted DiffDiff
Patch for committing (15.46 KB, patch)
2016-03-30 10:49 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1 2016-03-28 19:35:57 PDT
Created attachment 275077 [details] Patch
Myles C. Maxfield
Comment 2 2016-03-28 19:43:56 PDT
Created attachment 275078 [details] Patch
Simon Fraser (smfr)
Comment 3 2016-03-28 21:46:13 PDT
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().
Myles C. Maxfield
Comment 4 2016-03-29 10:14:03 PDT
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.
Myles C. Maxfield
Comment 5 2016-03-29 10:16:25 PDT
Committed r198784: <http://trac.webkit.org/changeset/198784>
Myles C. Maxfield
Comment 6 2016-03-29 17:38:07 PDT
Rolled out in https://bugs.webkit.org/show_bug.cgi?id=155999
Myles C. Maxfield
Comment 7 2016-03-29 19:46:02 PDT
Created attachment 275166 [details] Needs to disable Settings::setMockScrollbarsEnabled
Myles C. Maxfield
Comment 8 2016-03-29 21:27:27 PDT
Created attachment 275178 [details] Patch
Simon Fraser (smfr)
Comment 9 2016-03-29 21:44:34 PDT
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) ?
Darin Adler
Comment 10 2016-03-30 09:09:54 PDT
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.
Myles C. Maxfield
Comment 11 2016-03-30 10:44:14 PDT
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.
Myles C. Maxfield
Comment 12 2016-03-30 10:49:09 PDT
Created attachment 275201 [details] Patch for committing
Darin Adler
Comment 13 2016-03-30 11:34:09 PDT
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!
Myles C. Maxfield
Comment 14 2016-03-30 11:53:48 PDT
(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)
Myles C. Maxfield
Comment 15 2016-03-30 15:19:06 PDT
Committed r198859: <http://trac.webkit.org/changeset/198859>
David Kilzer (:ddkilzer)
Comment 16 2016-04-03 02:57:50 PDT
(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>
Myles C. Maxfield
Comment 18 2016-04-04 19:50:38 PDT
<rdar://problem/25118321>
Antonio Gomes
Comment 19 2016-04-04 22:16:04 PDT
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?
Myles C. Maxfield
Comment 20 2016-04-05 11:14:35 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.