Bug 107601 - Default mouse cursor behavior should be auto-hide for full screen video with custom controls
Summary: Default mouse cursor behavior should be auto-hide for full screen video with ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-22 17:20 PST by Jer Noble
Modified: 2017-03-30 17:30 PDT (History)
17 users (show)

See Also:

Attachments
WIP (8.00 KB, patch)
2013-01-22 17:20 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (19.93 KB, patch)
2013-01-23 12:21 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (27.29 KB, patch)
2013-02-22 16:40 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (27.26 KB, patch)
2013-03-04 16:13 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (28.30 KB, patch)
2013-03-04 16:25 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (28.25 KB, patch)
2013-03-04 16:52 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (66.46 KB, patch)
2013-03-04 18:20 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (66.27 KB, patch)
2013-03-04 18:40 PST, Jer Noble 		bdakin: review+
Details | Formatted Diff | Diff
Show Obsolete (7) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2013-01-22 17:20:00 PST 
Default mouse cursor behavior should be auto-hide for full screen video with custom controls
Comment 1 Jer Noble 2013-01-22 17:20:14 PST 
Created attachment 184089 [details]
WIP

WIP
Comment 2 Jer Noble 2013-01-23 12:21:45 PST 
Created attachment 184281 [details]
Patch
Comment 3 WebKit Review Bot 2013-01-23 12:26:39 PST 
Attachment 184281 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fullscreen/video-cursor-auto-hide-expected.txt', u'LayoutTests/fullscreen/video-cursor-auto-hide.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/fullscreen.css', u'Source/WebCore/html/shadow/MediaControls.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/EventHandler.h', u'Source/WebCore/page/FrameView.h', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/Settings.h', u'Source/WebCore/rendering/RenderTheme.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/testing/InternalSettings.cpp', u'Source/WebCore/testing/InternalSettings.h', u'Source/WebCore/testing/InternalSettings.idl']" exit_code: 1
Source/WebCore/rendering/style/RenderStyleConstants.h:415:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Jer Noble 2013-02-22 16:40:45 PST 
Created attachment 189860 [details]
Patch

-webkit-cursor-visibility: version.
Comment 5 WebKit Review Bot 2013-02-22 16:44:08 PST 
Attachment 189860 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fullscreen/video-cursor-auto-hide-expected.txt', u'LayoutTests/fullscreen/video-cursor-auto-hide.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleBuilder.cpp', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/fullscreen.css', u'Source/WebCore/html/shadow/MediaControls.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/EventHandler.h', u'Source/WebCore/page/FrameView.h', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/Settings.h', u'Source/WebCore/rendering/RenderTheme.h', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/testing/InternalSettings.cpp', u'Source/WebCore/testing/InternalSettings.h', u'Source/WebCore/testing/InternalSettings.idl']" exit_code: 1
Source/WebCore/rendering/style/RenderStyleConstants.h:422:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/RenderStyleConstants.h:423:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/RenderStyle.h:192:  _cursor_visibility_style is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Eric Carlson 2013-02-23 14:46:12 PST 
Comment on attachment 189860 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1845
> +        case CSSPropertyWebkitCursorVisibility: {
> +            return cssValuePool().createValue(style->cursorVisibility());
> +        }

These braces shouldn't be necessary.

> Source/WebCore/testing/InternalSettings.cpp:494
> +void InternalSettings::setTimeWithoutMouseMovementBeforeHidingControls(double time, ExceptionCode& ec)
> +{
> +    InternalSettingsGuardForSettings();
> +    settings()->setTimeWithoutMouseMovementBeforeHidingControls(time);
> +}

Should InternalSettings also save and restore the auto-hide time so one test doesn't change it for others?
Comment 7 Jer Noble 2013-02-24 10:24:50 PST 
(In reply to comment #6)
> (From update of attachment 189860 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189860&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1845
> > +        case CSSPropertyWebkitCursorVisibility: {
> > +            return cssValuePool().createValue(style->cursorVisibility());
> > +        }
> 
> These braces shouldn't be necessary.

I'll delete them.

> > Source/WebCore/testing/InternalSettings.cpp:494
> > +void InternalSettings::setTimeWithoutMouseMovementBeforeHidingControls(double time, ExceptionCode& ec)
> > +{
> > +    InternalSettingsGuardForSettings();
> > +    settings()->setTimeWithoutMouseMovementBeforeHidingControls(time);
> > +}
> 
> Should InternalSettings also save and restore the auto-hide time so one test doesn't change it for others?

Probably. :)  I'll upload a new patch.
Comment 8 Jer Noble 2013-03-04 16:13:03 PST 
Created attachment 191339 [details]
Patch

Added code to InternalSettings::Backup which will restore the original mouse movement timeout.
Comment 9 WebKit Review Bot 2013-03-04 16:18:18 PST 
Attachment 191339 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fullscreen/video-cursor-auto-hide-expected.txt', u'LayoutTests/fullscreen/video-cursor-auto-hide.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleBuilder.cpp', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/fullscreen.css', u'Source/WebCore/html/shadow/MediaControls.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/EventHandler.h', u'Source/WebCore/page/FrameView.h', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/Settings.h', u'Source/WebCore/rendering/RenderTheme.h', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/testing/InternalSettings.cpp', u'Source/WebCore/testing/InternalSettings.h', u'Source/WebCore/testing/InternalSettings.idl']" exit_code: 1
Source/WebCore/rendering/style/RenderStyleConstants.h:422:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/RenderStyleConstants.h:423:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/RenderStyle.h:192:  _cursor_visibility_style is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Jer Noble 2013-03-04 16:25:48 PST 
Created attachment 191341 [details]
Patch

'The braces remain' was the name of my high-school band.
Comment 11 Beth Dakin 2013-03-04 16:33:56 PST 
Comment on attachment 191339 [details]
Patch

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

It seems like this CSS property could be set on any element instead of just a video. Is that okay?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1847
> +        case CSSPropertyWebkitCursorVisibility: {

I don't think these braces are needed.

>> Source/WebCore/rendering/style/RenderStyle.h:192
>> +        unsigned _cursor_visibility_style : 1; // ECursorVisibility
> 
> _cursor_visibility_style is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

The underscore thing, though it matches _cursor_style, does seem like it is probably from another era of style guidelines. I would change this to honor the StyleBot's wishes unless the underscore thing is really the new hotness.

> Source/WebCore/rendering/style/RenderStyleConstants.h:421
> +enum ECursorVisibility {

Starting an enum with an E is also pretty old school. I'd just call this CursorVisibility instead.
Comment 12 WebKit Review Bot 2013-03-04 16:36:23 PST 
Attachment 191341 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fullscreen/video-cursor-auto-hide-expected.txt', u'LayoutTests/fullscreen/video-cursor-auto-hide.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleBuilder.cpp', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/fullscreen.css', u'Source/WebCore/html/shadow/MediaControls.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/EventHandler.h', u'Source/WebCore/page/FrameView.h', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/Settings.h', u'Source/WebCore/rendering/RenderTheme.h', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/testing/InternalSettings.cpp', u'Source/WebCore/testing/InternalSettings.h', u'Source/WebCore/testing/InternalSettings.idl']" exit_code: 1
Source/WebCore/rendering/style/RenderStyleConstants.h:422:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/RenderStyleConstants.h:423:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/RenderStyle.h:192:  _cursor_visibility_style is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Jer Noble 2013-03-04 16:48:31 PST 
(In reply to comment #11)
> (From update of attachment 191339 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191339&action=review
> 
> It seems like this CSS property could be set on any element instead of just a video. Is that okay?

Yeah.  It genuinely seems useful for non-<video> cases; video games, video-like overlays, <canvas>, etc.

> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1847
> > +        case CSSPropertyWebkitCursorVisibility: {
> 
> I don't think these braces are needed.

Indeed they're not. (See band name joke above.)

> >> Source/WebCore/rendering/style/RenderStyle.h:192
> >> +        unsigned _cursor_visibility_style : 1; // ECursorVisibility
> > 
> > _cursor_visibility_style is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> 
> The underscore thing, though it matches _cursor_style, does seem like it is probably from another era of style guidelines. I would change this to honor the StyleBot's wishes unless the underscore thing is really the new hotness.

I will honor StyleBot.

> > Source/WebCore/rendering/style/RenderStyleConstants.h:421
> > +enum ECursorVisibility {
> 
> Starting an enum with an E is also pretty old school. I'd just call this CursorVisibility instead.

I hear it's coming back though.  Like jellies!  (I'll rename it)
Comment 14 Jer Noble 2013-03-04 16:52:14 PST 
Created attachment 191354 [details]
Patch

Renamed: ECursorVisibility -> CursorVisibility, _cursor_visibility_style -> m_cursorVisibility.
Comment 15 WebKit Review Bot 2013-03-04 16:57:07 PST 
Attachment 191354 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fullscreen/video-cursor-auto-hide-expected.txt', u'LayoutTests/fullscreen/video-cursor-auto-hide.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleBuilder.cpp', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/fullscreen.css', u'Source/WebCore/html/shadow/MediaControls.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/EventHandler.h', u'Source/WebCore/page/FrameView.h', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/Settings.h', u'Source/WebCore/rendering/RenderTheme.h', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/testing/InternalSettings.cpp', u'Source/WebCore/testing/InternalSettings.h', u'Source/WebCore/testing/InternalSettings.idl']" exit_code: 1
Source/WebCore/rendering/style/RenderStyleConstants.h:422:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/RenderStyleConstants.h:423:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Jer Noble 2013-03-04 18:20:46 PST 
Created attachment 191371 [details]
Patch

Wrapped implementation in ENABLE(CURSOR_VISIBILITY) checks, enabled on mac.
Comment 17 Jer Noble 2013-03-04 18:40:48 PST 
Created attachment 191373 [details]
Patch

Rebaselined.
Comment 18 WebKit Review Bot 2013-03-04 18:44:25 PST 
Attachment 191373 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fullscreen/video-cursor-auto-hide-expected.txt', u'LayoutTests/fullscreen/video-cursor-auto-hide.html', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/FeatureDefines.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/Configurations/FeatureDefines.xcconfig', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleBuilder.cpp', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/fullscreen.css', u'Source/WebCore/html/shadow/MediaControls.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/EventHandler.h', u'Source/WebCore/page/FeatureObserver.h', u'Source/WebCore/page/FrameView.h', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/page/Settings.h', u'Source/WebCore/rendering/RenderTheme.h', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/testing/InternalSettings.cpp', u'Source/WebCore/testing/InternalSettings.h', u'Source/WebCore/testing/InternalSettings.idl', u'Source/WebKit/ChangeLog', u'Source/WebKit/WebKit.vcxproj/FeatureDefines.props', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/Configurations/FeatureDefines.xcconfig', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Configurations/FeatureDefines.xcconfig']" exit_code: 1
Source/WebCore/rendering/style/RenderStyleConstants.h:423:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/rendering/style/RenderStyleConstants.h:424:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Beth Dakin 2013-03-05 16:41:09 PST 
Comment on attachment 191373 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:75
> +#include "RenderTheme.h"

Do you need this?

> Source/WebCore/rendering/style/RenderStyleConstants.h:422
> +enum CursorVisibility {

I agree with StyleBot here!! So, CursorVisibilityAuto and CursorVisibilityAutoHide.
Comment 20 Jer Noble 2013-03-05 16:50:51 PST 
Comment on attachment 191373 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:75
>> +#include "RenderTheme.h"
> 
> Do you need this?

Ah! this is left over from when timeWithoutMouseMovementBeforeHidingControls() was accessed through RenderTheme rather than Settings. I'll delete it.

>> Source/WebCore/rendering/style/RenderStyleConstants.h:422
>> +enum CursorVisibility {
> 
> I agree with StyleBot here!! So, CursorVisibilityAuto and CursorVisibilityAutoHide.

I live to serve StyleBot.
Comment 21 Jer Noble 2013-03-06 16:30:06 PST 
Committed r145003: <http://trac.webkit.org/changeset/145003>