Bug 71490 – Support uiStateTransitionProgress for scrollbars

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

<rdar://problem/9849612>

ScrollAnimatorMac should also support uiStateTransitionProgress animations.

[Beth Dakin]

Created attachment 113535 [details]
Patch

Comment on attachment 113535 [details]
Patch

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

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:54
> +static bool supportsUIStateTransitionProgress()
> +{
> +    // FIXME: This is temportary until all platforms that support ScrollbarPainter support this part of the api.
> +    static ScrollbarPainter dummyPainter = [NSClassFromString(@"NSScrollerImp") scrollerImpWithStyle:wkRecommendedScrollerStyle() controlSize:RegularScrollbar horizontal:NO replacingScrollerImp:nil];
> +    static bool globalSupportsUIStateTransitionProgress = [dummyPainter respondsToSelector:@selector(mouseEnteredScroller)];
> +    return globalSupportsUIStateTransitionProgress;
> +}

You don’t need to instantiate an NSScrollerImp. You can use the class method +instancesRespondToSelector:, something along the lines of
    [NSClassFromString(@"NSScrollerImg") instancesRespondToSelector:@selector(mouseEnteredScroller)]

[Beth Dakin]

(In reply to comment #2)
> (From update of attachment 113535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113535&action=review
> 
> > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:54
> > +static bool supportsUIStateTransitionProgress()
> > +{
> > +    // FIXME: This is temportary until all platforms that support ScrollbarPainter support this part of the api.
> > +    static ScrollbarPainter dummyPainter = [NSClassFromString(@"NSScrollerImp") scrollerImpWithStyle:wkRecommendedScrollerStyle() controlSize:RegularScrollbar horizontal:NO replacingScrollerImp:nil];
> > +    static bool globalSupportsUIStateTransitionProgress = [dummyPainter respondsToSelector:@selector(mouseEnteredScroller)];
> > +    return globalSupportsUIStateTransitionProgress;
> > +}
> 
> You don’t need to instantiate an NSScrollerImp. You can use the class method +instancesRespondToSelector:, something along the lines of
>     [NSClassFromString(@"NSScrollerImg") instancesRespondToSelector:@selector(mouseEnteredScroller)]

Excellent! I will fix that now.

[Daniel Bates]

Comment on attachment 113535 [details]
Patch

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

I only briefly looked at this patch and didn't check it for correctness. I have some minor nits. None are show stoppers.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:50
> +    // FIXME: This is temportary until all platforms that support ScrollbarPainter support this part of the api.

Nit:
temportary => temporary
api => API

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:327
> +    default:
> +        ASSERT_NOT_REACHED();

Can we make this switch-statement stronger? In particular, can we omit the default case statement here so as to have the the compiler ensure that we handle all the enum values in FeatureToAnimate? The benefit of this is that we would catch a missing case statement at compile-time as opposed to run-time.

[Beth Dakin]

(In reply to comment #4)
> (From update of attachment 113535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113535&action=review
> 
> I only briefly looked at this patch and didn't check it for correctness. I have some minor nits. None are show stoppers.
> 
> > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:50
> > +    // FIXME: This is temportary until all platforms that support ScrollbarPainter support this part of the api.
> 
> Nit:
> temportary => temporary
> api => API
> 

Ah, good catch. Will fix.

> > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:327
> > +    default:
> > +        ASSERT_NOT_REACHED();
> 
> Can we make this switch-statement stronger? In particular, can we omit the default case statement here so as to have the the compiler ensure that we handle all the enum values in FeatureToAnimate? The benefit of this is that we would catch a missing case statement at compile-time as opposed to run-time.

Sure, that seems like a good idea.

[Beth Dakin]

Created attachment 113575 [details]
Patch with a few small adjustments

Here's a patch that addresses Dan's and Dan's (heh!) comments from above.

[Daniel Bates]

Comment on attachment 113575 [details]
Patch with a few small adjustments

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

[Beth Dakin]

Created attachment 113898 [details]
Patch that should pass mac-ews

[Sam Weinig]

Comment on attachment 113898 [details]
Patch that should pass mac-ews

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

> Source/WebCore/page/EventHandler.cpp:1716
> +        if (ScrollableArea* scrollableArea = m_lastScrollbarUnderMouse->scrollableArea())
> +            scrollableArea->scrollAnimator()->mouseExitedScrollbar(m_lastScrollbarUnderMouse.get());

This seems like something that should be communicated directly to the Scrollbar.  Scrollbar already has mouseMoved and other fun.

[Beth Dakin]

(In reply to comment #9)
> (From update of attachment 113898 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113898&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:1716
> > +        if (ScrollableArea* scrollableArea = m_lastScrollbarUnderMouse->scrollableArea())
> > +            scrollableArea->scrollAnimator()->mouseExitedScrollbar(m_lastScrollbarUnderMouse.get());
> 
> This seems like something that should be communicated directly to the Scrollbar.  Scrollbar already has mouseMoved and other fun.

Do you mean that you think that this code that calls into ScrollAnimator should be in Scrollbar instead of EventHandler? Or do you mean that you think that we shouldn't call into ScrollAnimator at all? Assuming you mean the first, that seems like a fine move.

[Beth Dakin]

Created attachment 113940 [details]
Patch that moves work to Scrollbar from EventHandler

Here's a patch that addresses Sam's comment and also fixes a bug with the patch that I noticed while doing that. It is necessary to hit test before we send the mouseExited() notification during a mouseUp event, so this patch has Scrollbar::mouseUp() take a PlatformMouseEvent so that it can do just that.

[Beth Dakin]

Thanks, Sam! Committed change with revision 99493.