Bug 107601

Summary: Default mouse cursor behavior should be auto-hide for full screen video with custom controls
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: alcinnz, allan.jensen, bdakin, benjamin, cmarcelo, eoconnor, eric.carlson, eric, esprehn+autocc, feature-media-reviews, graouts, gyuyoung.kim, macpherson, menard, ojan.autocc, rakuco, syoichi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=180247
Attachments:
Description Flags
WIP
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch bdakin: review+

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>
Comment 22 Antoine Quint 2017-12-01 09:04:34 PST 
This caused https://bugs.webkit.org/show_bug.cgi?id=180247.