WebKit Bugzilla
Default mouse cursor behavior should be auto-hide for full screen video with custom controls
Created attachment 184089 [details] WIP WIP
Created attachment 184281 [details] Patch
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.
Created attachment 189860 [details] Patch -webkit-cursor-visibility: version.
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 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?
(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.
Created attachment 191339 [details] Patch Added code to InternalSettings::Backup which will restore the original mouse movement timeout.
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.
Created attachment 191341 [details] Patch 'The braces remain' was the name of my high-school band.
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.
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.
(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)
Created attachment 191354 [details] Patch Renamed: ECursorVisibility -> CursorVisibility, _cursor_visibility_style -> m_cursorVisibility.
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.
Created attachment 191371 [details] Patch Wrapped implementation in ENABLE(CURSOR_VISIBILITY) checks, enabled on mac.
Created attachment 191373 [details] Patch Rebaselined.
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 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 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.
Committed r145003: <http://trac.webkit.org/changeset/145003>