WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
107601
Default mouse cursor behavior should be auto-hide for full screen video with custom controls
https://bugs.webkit.org/show_bug.cgi?id=107601
Summary
Default mouse cursor behavior should be auto-hide for full screen video with ...
Jer Noble
Reported
2013-01-22 17:20:00 PST
Default mouse cursor behavior should be auto-hide for full screen video with custom controls
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 attachment
proposed patch, testcase, etc.
Jer Noble
Comment 1
2013-01-22 17:20:14 PST
Created
attachment 184089
[details]
WIP WIP
Jer Noble
Comment 2
2013-01-23 12:21:45 PST
Created
attachment 184281
[details]
Patch
WebKit Review Bot
Comment 3
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.
Jer Noble
Comment 4
2013-02-22 16:40:45 PST
Created
attachment 189860
[details]
Patch -webkit-cursor-visibility: version.
WebKit Review Bot
Comment 5
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.
Eric Carlson
Comment 6
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?
Jer Noble
Comment 7
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.
Jer Noble
Comment 8
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.
WebKit Review Bot
Comment 9
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.
Jer Noble
Comment 10
2013-03-04 16:25:48 PST
Created
attachment 191341
[details]
Patch 'The braces remain' was the name of my high-school band.
Beth Dakin
Comment 11
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.
WebKit Review Bot
Comment 12
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.
Jer Noble
Comment 13
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)
Jer Noble
Comment 14
2013-03-04 16:52:14 PST
Created
attachment 191354
[details]
Patch Renamed: ECursorVisibility -> CursorVisibility, _cursor_visibility_style -> m_cursorVisibility.
WebKit Review Bot
Comment 15
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.
Jer Noble
Comment 16
2013-03-04 18:20:46 PST
Created
attachment 191371
[details]
Patch Wrapped implementation in ENABLE(CURSOR_VISIBILITY) checks, enabled on mac.
Jer Noble
Comment 17
2013-03-04 18:40:48 PST
Created
attachment 191373
[details]
Patch Rebaselined.
WebKit Review Bot
Comment 18
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.
Beth Dakin
Comment 19
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.
Jer Noble
Comment 20
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.
Jer Noble
Comment 21
2013-03-06 16:30:06 PST
Committed
r145003
: <
http://trac.webkit.org/changeset/145003
>
Antoine Quint
Comment 22
2017-12-01 09:04:34 PST
This caused
https://bugs.webkit.org/show_bug.cgi?id=180247
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug