Bug 104173 - Display the auto-start label image after a delay
Summary: Display the auto-start label image after a delay
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.8
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on:
Blocks: 98318
  Show dependency treegraph
 
Reported: 2012-12-05 14:48 PST by Jon Lee
Modified: 2012-12-07 14:10 PST (History)
10 users (show)

See Also:


Attachments
Patch (6.99 KB, patch)
2012-12-05 16:42 PST, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (7.78 KB, patch)
2012-12-06 16:08 PST, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (25.57 KB, patch)
2012-12-07 10:33 PST, Jon Lee
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2012-12-05 14:48:06 PST
Instead of showing the image right on hover, wait a little while beforehand.
Comment 1 Radar WebKit Bug Importer 2012-12-05 14:48:32 PST
<rdar://problem/12820071>
Comment 2 Jon Lee 2012-12-05 16:42:22 PST
Created attachment 177864 [details]
Patch
Comment 3 mitz 2012-12-05 19:22:36 PST
Comment on attachment 177864 [details]
Patch

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

I suspect this implementation is relying on nothing else causing a repaint during the 1-second delay. If anything on the page caused the plug-in to repaint during that time, then it would paint with the button before the time is up. Also, the user experience of hovering the plug-in for half a second, clicking in the (still invisible) button rect, only to reveal that you have activated a button that wasn’t there when you clicked seems like it could be confusing.

Perhaps there should be a dedicated boolean tracking whether the button should be visible?

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:59
> +    if (m_hoverDelayTimer.isActive())
> +        m_hoverDelayTimer.stop();

The DeferrableOneShotTimer is a member variable, so it will be destroyed and therefore stopped here anyway.

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:208
>          }

If I mouse down inside the button, then drag out of the button (but still inside the plug-in), then release, won’t the button maintain the pressed appearance?

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:210
> +        if (m_hoverDelayTimer.isActive())

How can this be true?

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:219
> +        event->setDefaultHandled();

Maybe the diff is confusing me, but it looks like you’re calling setDefaultEnabled() twice for mouseout.
Comment 4 Jon Lee 2012-12-06 15:55:12 PST
Comment on attachment 177864 [details]
Patch

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

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:59
>> +        m_hoverDelayTimer.stop();
> 
> The DeferrableOneShotTimer is a member variable, so it will be destroyed and therefore stopped here anyway.

Removed.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:208
>>          }
> 
> If I mouse down inside the button, then drag out of the button (but still inside the plug-in), then release, won’t the button maintain the pressed appearance?

Yes. This is similar to input buttons, I think. But I will amend this to let the click event prevent starting the plugin if you move the mouse away from the button.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:210
>> +        if (m_hoverDelayTimer.isActive())
> 
> How can this be true?

It shouldn't be true. Removed. I think this got mixed in when I was refactoring.

>> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:219
>> +        event->setDefaultHandled();
> 
> Maybe the diff is confusing me, but it looks like you’re calling setDefaultEnabled() twice for mouseout.

Nice catch! Removed.
Comment 5 Jon Lee 2012-12-06 16:08:22 PST
Created attachment 178100 [details]
Patch
Comment 6 Jon Lee 2012-12-07 10:33:37 PST
Created attachment 178237 [details]
Patch
Comment 7 mitz 2012-12-07 13:51:03 PST
Comment on attachment 178237 [details]
Patch

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

> Source/WebCore/rendering/RenderSnapshottedPlugIn.h:66
> +    bool m_showLabel;

I think m_showsLabel, m_isShowingLabel or m_shouldShowLabel are all better names for this.
Comment 8 Jon Lee 2012-12-07 14:09:35 PST
Committed r136982: <http://trac.webkit.org/changeset/136982>
Comment 9 Jon Lee 2012-12-07 14:10:47 PST
I opted for m_shouldShowLabel. Thanks!