Bug 107601 - Default mouse cursor behavior should be auto-hide for full screen video with custom controls
: Default mouse cursor behavior should be auto-hide for full screen video with ...
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2013-01-22 17:20 PST by
Modified: 2013-03-06 16:30 PST (History)


Attachments
WIP (8.00 KB, patch)
2013-01-22 17:20 PST, Jer Noble
no flags Review Patch | Details | Formatted Diff | Diff
Patch (19.93 KB, patch)
2013-01-23 12:21 PST, Jer Noble
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.29 KB, patch)
2013-02-22 16:40 PST, Jer Noble
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.26 KB, patch)
2013-03-04 16:13 PST, Jer Noble
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.30 KB, patch)
2013-03-04 16:25 PST, Jer Noble
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.25 KB, patch)
2013-03-04 16:52 PST, Jer Noble
no flags Review Patch | Details | Formatted Diff | Diff
Patch (66.46 KB, patch)
2013-03-04 18:20 PST, Jer Noble
no flags Review Patch | Details | Formatted Diff | Diff
Patch (66.27 KB, patch)
2013-03-04 18:40 PST, Jer Noble
bdakin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2013-01-22 17:20:00 PST
Default mouse cursor behavior should be auto-hide for full screen video with custom controls
------- Comment #1 From 2013-01-22 17:20:14 PST -------
Created an attachment (id=184089) [details]
WIP

WIP
------- Comment #2 From 2013-01-23 12:21:45 PST -------
Created an attachment (id=184281) [details]
Patch
------- Comment #3 From 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 From 2013-02-22 16:40:45 PST -------
Created an attachment (id=189860) [details]
Patch

-webkit-cursor-visibility: version.
------- Comment #5 From 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 From 2013-02-23 14:46:12 PST -------
(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.

> 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 From 2013-02-24 10:24:50 PST -------
(In reply to comment #6)
> (From update of attachment 189860 [details] [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 From 2013-03-04 16:13:03 PST -------
Created an attachment (id=191339) [details]
Patch

Added code to InternalSettings::Backup which will restore the original mouse movement timeout.
------- Comment #9 From 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 From 2013-03-04 16:25:48 PST -------
Created an attachment (id=191341) [details]
Patch

'The braces remain' was the name of my high-school band.
------- Comment #11 From 2013-03-04 16:33:56 PST -------
(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?

> 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 From 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 From 2013-03-04 16:48:31 PST -------
(In reply to comment #11)
> (From update of attachment 191339 [details] [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 From 2013-03-04 16:52:14 PST -------
Created an attachment (id=191354) [details]
Patch

Renamed: ECursorVisibility -> CursorVisibility, _cursor_visibility_style -> m_cursorVisibility.
------- Comment #15 From 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 From 2013-03-04 18:20:46 PST -------
Created an attachment (id=191371) [details]
Patch

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

Rebaselined.
------- Comment #18 From 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 From 2013-03-05 16:41:09 PST -------
(From update of attachment 191373 [details])
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 From 2013-03-05 16:50:51 PST -------
(From update of attachment 191373 [details])
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 From 2013-03-06 16:30:06 PST -------
Committed r145003: <http://trac.webkit.org/changeset/145003>