Bug 107601 – Default mouse cursor behavior should be auto-hide for full screen video with custom controls

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Default mouse cursor behavior should be auto-hide for full screen video with custom controls

[Jer Noble]

Created attachment 184089 [details]
WIP

WIP

[Jer Noble]

Created attachment 184281 [details]
Patch

[WebKit Review Bot]

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.

[Jer Noble]

Created attachment 189860 [details]
Patch

-webkit-cursor-visibility: version.

[WebKit Review Bot]

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.

[Eric Carlson]

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?

[Jer Noble]

(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.

[Jer Noble]

Created attachment 191339 [details]
Patch

Added code to InternalSettings::Backup which will restore the original mouse movement timeout.

[WebKit Review Bot]

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.

[Jer Noble]

Created attachment 191341 [details]
Patch

'The braces remain' was the name of my high-school band.

[Beth Dakin]

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.

[WebKit Review Bot]

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.

[Jer Noble]

(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)

[Jer Noble]

Created attachment 191354 [details]
Patch

Renamed: ECursorVisibility -> CursorVisibility, _cursor_visibility_style -> m_cursorVisibility.

[WebKit Review Bot]

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.

[Jer Noble]

Created attachment 191371 [details]
Patch

Wrapped implementation in ENABLE(CURSOR_VISIBILITY) checks, enabled on mac.

[Jer Noble]

Created attachment 191373 [details]
Patch

Rebaselined.

[WebKit Review Bot]

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.

[Beth Dakin]

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.

[Jer Noble]

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.

[Jer Noble]

Committed r145003: <http://trac.webkit.org/changeset/145003>

[Antoine Quint]

This caused https://bugs.webkit.org/show_bug.cgi?id=180247.