Bug 104173 – Display the auto-start label image after a delay

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jon Lee]

Instead of showing the image right on hover, wait a little while beforehand.

[Radar WebKit Bug Importer]

<rdar://problem/12820071>

[Jon Lee]

Created attachment 177864 [details]
Patch

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.

[Jon Lee]

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.

[Jon Lee]

Created attachment 178100 [details]
Patch

[Jon Lee]

Created attachment 178237 [details]
Patch

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.

[Jon Lee]

Committed r136982: <http://trac.webkit.org/changeset/136982>

[Jon Lee]

I opted for m_shouldShowLabel. Thanks!