Bug 75904 – WebKit 1: Scrollbar uiStateTransitionProgress requires tracking the mouse all the time

• Component: Platform
• Status: RESOLVED FIXED

[Beth Dakin]

With https://bugs.webkit.org/show_bug.cgi?id=71490 and http://trac.webkit.org/changeset/99493 I added support for uiStateTransitionProgress on scrollbars. To properly animate uiStateTransitionProgress, we keep track of whenever the mouse enters or exits a scrollbar. However, to fully support the feature, we have to keep track of that information even when the window is not key. Currently, we only track mouseMoved events when the window is key, so this needs to change to fully support uiStateTransitionProgress. This bug has already been fixed in WebKit 2 (see https://bugs.webkit.org/show_bug.cgi?id=72400 ), so this report is about fixing it for WebKit 1.

<rdar://problem/10498816>

[Beth Dakin]

Created attachment 121756 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 121756 [details]
Patch 

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

I have a few comments, but I'm going to let someone who's more knowledgeable about WebHTMLView give the official r+.

> Source/WebCore/ChangeLog:5
> +        WebKit 1: Scrollbar uiStateTransitionProgress requires tracking the mouse all the 
> +        time

Is there a way to title this bug in terms of the incorrect behavior that users will see without this patch?

> Source/WebCore/page/EventHandler.h:199
> +    void mouseMoved(NSEvent *, bool onlyUpdateScrollbars = false);

Optional boolean parameters make me sad. They make refactoring (e.g., adding a new required argument before the optional boolean) hard, because so many types can be implicitly converted to bool. Would it cause lots of code churn to make the boolean non-optional?

Alternatively, perhaps it would be more appropriate to have a different public member function to handle this case. The fact that we update scrollbars via a fake mouseMoved event seems like an implementation detail.

> Source/WebKit/mac/ChangeLog:12
> +        Opt into a more-efficient code path when we know that only scrollbar updates will 
> +        have any effect. 

This is confusingly worded. I think you mean "…when we know that mouse movements won't affect anything other than scrollbars", or something like that.

> Source/WebKit/mac/WebView/WebHTMLView.mm:1609
> +        // Lion when legacy scrollbars are enabled, WebKit receives mouse events all the time. If it is one

"Lion" should probably be "Lion and newer".

> Source/WebKit/mac/WebView/WebHTMLView.mm:3374
> +    NSArray *trackingAreas = [self trackingAreas];
> +    NSUInteger count = [trackingAreas count];
> +    ASSERT(count <= 1);
> +
> +    // Remove any tracking areas. These are only currently used to track legacy scrollbars
> +    // when the window is not key.
> +    for (NSUInteger i = 0; i < count; ++i)
> +        [self removeTrackingArea:[trackingAreas objectAtIndex:i]];

Maybe it would be simpler to store the tracking area in an ivar so that we can remove it directly, rather than removing all tracking areas?

[Beth Dakin]

(In reply to comment #2)
> (From update of attachment 121756 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121756&action=review
> 
> I have a few comments, but I'm going to let someone who's more knowledgeable about WebHTMLView give the official r+.
> 
> > Source/WebCore/ChangeLog:5
> > +        WebKit 1: Scrollbar uiStateTransitionProgress requires tracking the mouse all the 
> > +        time
> 
> Is there a way to title this bug in terms of the incorrect behavior that users will see without this patch?
> 

Not at this time, unfortunately.

> > Source/WebCore/page/EventHandler.h:199
> > +    void mouseMoved(NSEvent *, bool onlyUpdateScrollbars = false);
> 
> Optional boolean parameters make me sad. They make refactoring (e.g., adding a new required argument before the optional boolean) hard, because so many types can be implicitly converted to bool. Would it cause lots of code churn to make the boolean non-optional?
> 

It actually doesn't seem like it would be a huge amount of churn to make the parameter non-optional, so I will post a new patch to do just that.

> Alternatively, perhaps it would be more appropriate to have a different public member function to handle this case. The fact that we update scrollbars via a fake mouseMoved event seems like an implementation detail.
> 

That is true. I feel like we considered a different function entirely when I first added the parameter to the platform-independent mouseMoved(), but I can't remember why we decided not to do it that way. 

> > Source/WebKit/mac/ChangeLog:12
> > +        Opt into a more-efficient code path when we know that only scrollbar updates will 
> > +        have any effect. 
> 
> This is confusingly worded. I think you mean "…when we know that mouse movements won't affect anything other than scrollbars", or something like that.
> 

Will fix.

> > Source/WebKit/mac/WebView/WebHTMLView.mm:1609
> > +        // Lion when legacy scrollbars are enabled, WebKit receives mouse events all the time. If it is one
> 
> "Lion" should probably be "Lion and newer".
> 

Good point, will fix.

> > Source/WebKit/mac/WebView/WebHTMLView.mm:3374
> > +    NSArray *trackingAreas = [self trackingAreas];
> > +    NSUInteger count = [trackingAreas count];
> > +    ASSERT(count <= 1);
> > +
> > +    // Remove any tracking areas. These are only currently used to track legacy scrollbars
> > +    // when the window is not key.
> > +    for (NSUInteger i = 0; i < count; ++i)
> > +        [self removeTrackingArea:[trackingAreas objectAtIndex:i]];
> 
> Maybe it would be simpler to store the tracking area in an ivar so that we can remove it directly, rather than removing all tracking areas?

That seems like a good idea as well.

[Beth Dakin]

(In reply to comment #3)
> > > Source/WebCore/page/EventHandler.h:199
> > > +    void mouseMoved(NSEvent *, bool onlyUpdateScrollbars = false);
> > 
> > Optional boolean parameters make me sad. They make refactoring (e.g., adding a new required argument before the optional boolean) hard, because so many types can be implicitly converted to bool. Would it cause lots of code churn to make the boolean non-optional?
> > 
> 
> It actually doesn't seem like it would be a huge amount of churn to make the parameter non-optional, so I will post a new patch to do just that.
> 
> > Alternatively, perhaps it would be more appropriate to have a different public member function to handle this case. The fact that we update scrollbars via a fake mouseMoved event seems like an implementation detail.
> > 

I've come all the way around, and I now think that it's more appropriate to have a different public member function. There isn't a huge quantity of churn to make the parameter non-optional, but adding it just makes so little sense in the places where it needs to be added that it really highlights the need for a separate function entirely.

[Beth Dakin]

Created attachment 122104 [details]
New patch

[WebKit Review Bot]

Attachment 122104 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1211:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 122104 [details]
New patch

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

r=me, but I think there are some things that need to be improved after check-in, if not before

> Source/WebCore/page/EventHandler.cpp:1585
> +    HitTestResult hoveredNode = HitTestResult(LayoutPoint());

There’s no reason to include the part after the equal sign. The default constructor does the same thing.

> Source/WebCore/page/EventHandler.cpp:1586
> +    return handleMouseMoveEvent(event, &hoveredNode, true);

A boolean is not good when you are passing a constant value. This kind of situation leads us to use enums or refactor so function can be separate.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1211
> +            if (performFullHitTest)
> +                return frame->eventHandler()->mouseMoved(platformMouseEvent);
> +            else
> +                return frame->eventHandler()->passMouseMovedEventToScrollbars(platformMouseEvent);

We normally don’t do else after return.

The name “perform full hit test” doesn’t seem to express the entire difference here between “pass event to everything” and “pass event to scrollbars”.

> Source/WebKit/mac/WebView/WebHTMLView.mm:1553
> +static bool mouseButtonIsPressedForEvent(NSEvent *event)

I think this is not quite the right name. After all, the mouse button is not pressed for a mouse up event. It’s more that this event is “part of a click or drag”. Sorry, I don’t have a great name to suggest.

> Source/WebKit/mac/WebView/WebHTMLView.mm:1617
> +            // If it is one of those cases where the page is not active and the mouse is not pressed, then we can
> +            // fire a more efficient scrollbars-only version of the event.

The issue is not just one of efficiency, so I think the comment is misleading in this regard.

There are things we do not want to do when the mouse passes over the window and it’s not frontmost. For example, we don’t want to dispatch DOM events.

> Source/WebKit/mac/WebView/WebHTMLView.mm:3415
> +        // Legacy style scrollbars have design details that rely on tracking the mouse all the time.
> +        // It's easiest to do this with a tracking area, which we will remove when the window is key
> +        // again.
> +        _private->trackingAreaForNonKeyWindow = [[NSTrackingArea alloc] initWithRect:[self frame]
> +                                                                    options:NSTrackingMouseMoved | NSTrackingMouseEnteredAndExited | NSTrackingInVisibleRect | NSTrackingActiveAlways
> +                                                                      owner:self
> +                                                                   userInfo:nil];

I believe [self frame] is wrong for the rectangle. I think we want [self bounds], based on the documentation in NSTrackingArea. The code says that the rectangle is in the view’s coordinate system, and bounds is the rectangle of a view in its own coordinate system.

Better not to try to line the colons up. I am guessing that Xcode tried to do this automatically.

Why add the tracking area only when non-key, and keep creating and destroying it each time? Does the tracking area do any harm when the window is key?

[Beth Dakin]

(In reply to comment #7)
> (From update of attachment 122104 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122104&action=review
> 
> r=me, but I think there are some things that need to be improved after check-in, if not before
> 

Thank you!

> > Source/WebCore/page/EventHandler.cpp:1585
> > +    HitTestResult hoveredNode = HitTestResult(LayoutPoint());
> 
> There’s no reason to include the part after the equal sign. The default constructor does the same thing.
> 

Fixed.

> > Source/WebCore/page/EventHandler.cpp:1586
> > +    return handleMouseMoveEvent(event, &hoveredNode, true);
> 
> A boolean is not good when you are passing a constant value. This kind of situation leads us to use enums or refactor so function can be separate.
> 

I'm not sure which is better -- an enum or a separate function. I'm leaning toward an enum because there really would be a lot of duplicated lines of code if it was totally separate. I will try to think of a good enum name.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1211
> > +            if (performFullHitTest)
> > +                return frame->eventHandler()->mouseMoved(platformMouseEvent);
> > +            else
> > +                return frame->eventHandler()->passMouseMovedEventToScrollbars(platformMouseEvent);
> 
> We normally don’t do else after return.
> 

Ah, good catch. Style bot didn't like that either. Fixed.

> The name “perform full hit test” doesn’t seem to express the entire difference here between “pass event to everything” and “pass event to scrollbars”.
> 

True. I switched it back to the old variable name which tracked the opposite state -- onlyUpdateScrollbars.

> > Source/WebKit/mac/WebView/WebHTMLView.mm:1553
> > +static bool mouseButtonIsPressedForEvent(NSEvent *event)
> 
> I think this is not quite the right name. After all, the mouse button is not pressed for a mouse up event. It’s more that this event is “part of a click or drag”. Sorry, I don’t have a great name to suggest.
> 

I changed it to mouseEventIsPartOfClickOrDrag() which is a bit clunky, but at least more accurate.

> > Source/WebKit/mac/WebView/WebHTMLView.mm:1617
> > +            // If it is one of those cases where the page is not active and the mouse is not pressed, then we can
> > +            // fire a more efficient scrollbars-only version of the event.
> 
> The issue is not just one of efficiency, so I think the comment is misleading in this regard.
> 
> There are things we do not want to do when the mouse passes over the window and it’s not frontmost. For example, we don’t want to dispatch DOM events.
> 

Good point. I improved the comment.

> > Source/WebKit/mac/WebView/WebHTMLView.mm:3415
> > +        // Legacy style scrollbars have design details that rely on tracking the mouse all the time.
> > +        // It's easiest to do this with a tracking area, which we will remove when the window is key
> > +        // again.
> > +        _private->trackingAreaForNonKeyWindow = [[NSTrackingArea alloc] initWithRect:[self frame]
> > +                                                                    options:NSTrackingMouseMoved | NSTrackingMouseEnteredAndExited | NSTrackingInVisibleRect | NSTrackingActiveAlways
> > +                                                                      owner:self
> > +                                                                   userInfo:nil];
> 
> I believe [self frame] is wrong for the rectangle. I think we want [self bounds], based on the documentation in NSTrackingArea. The code says that the rectangle is in the view’s coordinate system, and bounds is the rectangle of a view in its own coordinate system.
> 

Fixed.

> Better not to try to line the colons up. I am guessing that Xcode tried to do this automatically.
> 

Fixed.

> Why add the tracking area only when non-key, and keep creating and destroying it each time? Does the tracking area do any harm when the window is key?

I am concerned about getting multiple messages from AppKit through both the tracking area and our mouse moved observers. I don't know for sure that AppKit would make that mistake of sending us the message through both mechanisms, but I also don't know a good way to test it since it is so tricky to debug event code. Let me know if you have any good ideas for testing this.

[Beth Dakin]

Darin and I discussed this in person and decided that handleMouseMoveEvent() should be refactored in a future patch so that it doesn't have the onlyUpdateScrollbars parameter at all. All of that work should be moved to the new passMouseMovedEventToScrollbars().

This patch is committed with http://trac.webkit.org/changeset/104773