155962 – [OS X] [RTL Scrollbars] Overlay RTL scrollbars animate in from the wrong side

RESOLVED FIXED 155962

[OS X] [RTL Scrollbars] Overlay RTL scrollbars animate in from the wrong side

https://bugs.webkit.org/show_bug.cgi?id=155962

Summary [OS X] [RTL Scrollbars] Overlay RTL scrollbars animate in from the wrong side

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	Details Formatted Diff Diff
Patch (8.53 KB, patch) 2016-03-28 19:43 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Needs to disable Settings::setMockScrollbarsEnabled (10.30 KB, patch) 2016-03-29 19:46 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Patch (15.50 KB, patch) 2016-03-29 21:27 PDT, Myles C. Maxfield	darin: review+	Details Formatted Diff Diff
Patch for committing (15.46 KB, patch) 2016-03-30 10:49 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
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> 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> > > 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>

David Kilzer (:ddkilzer)

Comment 17 2016-04-03 02:59:13 PDT

(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>

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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

Myles C. Maxfield

Reported

2016-03-28 19:29 PDT

Modified

2016-04-05 11:14 PDT History

CC List

13 users Show

URL

Keywords InRadar

Depends on

155999

Blocks

Dependencies

tree graph