New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GTK][WPE] Enable modern media controls #1147
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a ton for working on this @philn! 💪🏼
The JS/CSS for the media controls is a bit out of my zone of comfort... from what I understand the changes look fine; but I will let others comment on those.
I have a minor nit about packaging: it would be a good idea to run the concatenated JS/CSS through jsmin.py
/cssmin.py
, but as we were not doing it already before, feel free to skip that change and we can consider it for a follow-up patch. The rest of the CMake stuff looks great.
Lastly, I see that there are a few inconsistencies with the icons:
- The skip length is set to 15 in code, then the
SkipBack15.svg
icon has 30 written on it, andSkipForward15.svg
has 1 written on it. - The
Pause.svg
icon contains a “play” icon (▶️ , should be ⏸️). - The
Play.svg
icon contains a “pause” icon (⏸️, should be▶️ ).
Could you please take a look at the icons?
add_custom_command( | ||
OUTPUT ${WebCore_DERIVED_SOURCES_DIR}/ModernMediaControls.js | ||
DEPENDS ${MODERN_MEDIA_CONTROLS_SCRIPTS} | ||
COMMAND cat ${MODERN_MEDIA_CONTROLS_SCRIPTS} > ${WebCore_DERIVED_SOURCES_DIR}/ModernMediaControls.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to run the output from this command through Source/JavaScriptCore/Scripts/jsmin.py
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I tried this, the minifier renamed the iconService
constant, breaking everything.
add_custom_command( | ||
OUTPUT ${WebCore_DERIVED_SOURCES_DIR}/ModernMediaControls.css | ||
DEPENDS ${MODERN_MEDIA_CONTROLS_STYLE_SHEETS} | ||
COMMAND cat ${MODERN_MEDIA_CONTROLS_STYLE_SHEETS} > ${WebCore_DERIVED_SOURCES_DIR}/ModernMediaControls.css |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be piped through Source/JavaScriptCore/Scripts/cssmin.py
to reduce the size of the output for bundling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this, also broke the controls, no error was logged though.
Source/WebCore/Modules/modern-media-controls/controls/adwaita-layout-traits.js
Show resolved
Hide resolved
Yeah I thought about it too, let's handle this as a follow-up.
Right, I started editing those in Inkscape and realized I had no idea about design and no idea about which font to use for the numbers either. I'd need help from the GNOME designers about this...
That might look inconsistent, but it's the way the Apple controls work. |
Excellent!
I expect the font is Cantarell, but I can take a look and get you updated icons in the same style. Or maybe I can prepare a follow-up PR. Let me know what would be more convenient for you.
Ah, I think it probably |
@aperezdc you should be able to push to my fork, if you have some time to update the icons directly in the branch that would be most appreciated :) |
There's almost no chance that's intended. Better to just fix it. |
I guess you'll replace |
Done, I have amended the commit with fixed icons that have “15” in them. |
If you try this branch, you'll see this works as expected. |
? |
Yes, it works as intended, I also tried it while editing the icons 😄 |
Source/WebCore/Modules/modern-media-controls/media/media-document-controller.js
Outdated
Show resolved
Hide resolved
I was planning to do that in another patch, but yeah, they're broken anyway... so let's ditch those already. |
You should be able to kill https://webkit-search.igalia.com/webkit/rev/00d01db2338b1568d39aa439334f39f0353d0b00/Source/WebCore/platform/ThemeTypes.h#45-47 / https://webkit-search.igalia.com/webkit/rev/00d01db2338b1568d39aa439334f39f0353d0b00/Source/WebCore/platform/ThemeTypes.h#76-78 and related code. Let me know if you need any help with that. |
Ok but this patch is already quite big! I'd rather do this additional cleanup as a follow-up, if you don't mind :) |
Sure, just making note of it here so you don't forget :) |
https://bugs.webkit.org/show_bug.cgi?id=241219 Reviewed by Tim Nguyen. * metadata/contributors.json: Add my github account. Canonical link: https://commits.webkit.org/251218@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295127 268f45cc-cd09-0410-ab3c-d52691b4dbfc
https://bugs.webkit.org/show_bug.cgi?id=182502 Patch by Philippe Normand <philn@igalia.com> on 2022-06-02 Reviewed by Adrian Perez de Castro. Initial support for the modern media controls on GTK and WPE. The old media controls are broken, their buttons are no longer rendered. These new controls are adapted from the macOS controls, both inline and fullscreen, with the difference that we use the Adwaita icon theme. SVG/PNG icons are shipped in the WebKit library using GResources. Some adaptations had to be made in the cross-platform JS code of the controls, so that Airplay UI components can be disabled, WPE/GTK don't support this yet. * LayoutTests/platform/glib/TestExpectations: * LayoutTests/platform/gtk/TestExpectations: * Source/WTF/wtf/Platform.h: * Source/WebCore/CMakeLists.txt: * Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp: (WebCore::MediaControlsHost::layoutTraitsClassName const): * Source/WebCore/Modules/mediacontrols/mediaControlsAdwaita.css: Removed. * Source/WebCore/Modules/mediacontrols/mediaControlsAdwaita.js: Removed. * Source/WebCore/Modules/modern-media-controls/controls/adwaita-fullscreen-media-controls.css: Added. (.media-controls.adwaita.fullscreen): (.media-controls.adwaita.fullscreen > .controls-bar): (.media-controls.adwaita.fullscreen:not(.uses-ltr-user-interface-layout-direction) .volume.slider): (.media-controls.adwaita.fullscreen .buttons-container): (.media-controls.adwaita.fullscreen .buttons-container.left): (.media-controls.adwaita.fullscreen .buttons-container.center): (.media-controls.adwaita.fullscreen .buttons-container.right): (.media-controls.adwaita.fullscreen .buttons-container.right button): (.media-controls.adwaita.fullscreen .time-control): (.media-controls.adwaita.fullscreen > .controls-bar .status-label): * Source/WebCore/Modules/modern-media-controls/controls/adwaita-fullscreen-media-controls.js: Added. (AdwaitaFullscreenMediaControls.prototype.handleEvent): (AdwaitaFullscreenMediaControls.prototype.layout): (AdwaitaFullscreenMediaControls.prototype._volumeControlsForCurrentDirection): (AdwaitaFullscreenMediaControls.prototype._collapsableButtons): (AdwaitaFullscreenMediaControls.prototype._handleMousedown): (AdwaitaFullscreenMediaControls.prototype._handleMousemove): (AdwaitaFullscreenMediaControls.prototype._handleMouseup): (AdwaitaFullscreenMediaControls.prototype._pointForEvent): (AdwaitaFullscreenMediaControls): * Source/WebCore/Modules/modern-media-controls/controls/adwaita-inline-media-controls.css: Added. (.media-controls.adwaita.inline .volume-slider-container): (.media-controls.adwaita.inline.audio .volume-slider-container): (.media-controls.adwaita.inline .volume-slider-container > .background-tint): (.media-controls.adwaita.inline .volume-slider-container > .background-tint > div): (.media-controls.adwaita.inline .volume-slider-container > .slider): * Source/WebCore/Modules/modern-media-controls/controls/adwaita-inline-media-controls.js: Added. (AdwaitaInlineMediaControls.prototype.layout): (AdwaitaInlineMediaControls.prototype.get preferredMuteButtonStyle): (AdwaitaInlineMediaControls.prototype.handleEvent): * Source/WebCore/Modules/modern-media-controls/controls/adwaita-layout-traits.js: Copied from Source/WebCore/Modules/modern-media-controls/controls/macos-layout-traits.js. (AdwaitaLayoutTraits.prototype.mediaControlsClass): (AdwaitaLayoutTraits.prototype.overridenSupportingObjectClasses): (AdwaitaLayoutTraits.prototype.resourceDirectory): (AdwaitaLayoutTraits.prototype.controlsAlwaysAvailable): (AdwaitaLayoutTraits.prototype.controlsNeverAvailable): (AdwaitaLayoutTraits.prototype.supportsIconWithFullscreenVariant): (AdwaitaLayoutTraits.prototype.supportsDurationTimeLabel): (AdwaitaLayoutTraits.prototype.controlsDependOnPageScaleFactor): (AdwaitaLayoutTraits.prototype.skipDuration): (AdwaitaLayoutTraits.prototype.promoteSubMenusWhenShowingMediaControlsContextMenu): (AdwaitaLayoutTraits.prototype.supportsTouches): (AdwaitaLayoutTraits.prototype.supportsAirPlay): (AdwaitaLayoutTraits.prototype.supportsPiP): (AdwaitaLayoutTraits.prototype.toString): (AdwaitaLayoutTraits): * Source/WebCore/Modules/modern-media-controls/controls/inline-media-controls.js: (InlineMediaControls.prototype._rightContainerButtons): (InlineMediaControls.prototype._droppableButtons): * Source/WebCore/Modules/modern-media-controls/controls/ios-layout-traits.js: (IOSLayoutTraits.prototype.supportsAirPlay): (IOSLayoutTraits.prototype.supportsPiP): * Source/WebCore/Modules/modern-media-controls/controls/layout-traits.js: (LayoutTraits.prototype.supportsAirPlay): (LayoutTraits.prototype.supportsPiP): * Source/WebCore/Modules/modern-media-controls/controls/macos-layout-traits.js: (MacOSLayoutTraits.prototype.supportsAirPlay): (MacOSLayoutTraits.prototype.supportsPiP): * Source/WebCore/Modules/modern-media-controls/controls/media-controls.js: (MediaControls.): * Source/WebCore/Modules/modern-media-controls/controls/watchos-layout-traits.js: (WatchOSLayoutTraits.prototype.supportsAirPlay): (WatchOSLayoutTraits.prototype.supportsPiP): * Source/WebCore/Modules/modern-media-controls/images/adwaita/EnterFullscreen.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/ExitFullscreen.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Forward.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/MediaSelector-fullscreen.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/MediaSelector.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Overflow.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Pause.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/PipIn-fullscreen.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/PipIn.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/PipOut.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Play.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Rewind.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/SkipBack10.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/SkipBack15.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/SkipForward10.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/SkipForward15.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Volume0-RTL.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Volume0.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Volume1-RTL.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Volume1.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Volume2-RTL.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Volume2.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Volume3-RTL.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/Volume3.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/VolumeMuted-RTL.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/VolumeMuted.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/X.svg: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/invalid-placard@1x.png: Added. * Source/WebCore/Modules/modern-media-controls/images/adwaita/invalid-placard@2x.png: Added. * Source/WebCore/Modules/modern-media-controls/media/media-controller.js: (MediaController.prototype._supportingObjectClasses): * Source/WebCore/Modules/modern-media-controls/media/media-document-controller.js: (MediaDocumentController): * Source/WebCore/PlatformGLib.cmake: Added. * Source/WebCore/PlatformGTK.cmake: * Source/WebCore/PlatformMac.cmake: * Source/WebCore/PlatformWPE.cmake: * Source/WebCore/rendering/RenderThemeAdwaita.cpp: (WebCore::RenderThemeAdwaita::mediaControlsScripts): (WebCore::RenderThemeAdwaita::mediaControlsStyleSheet): (WebCore::RenderThemeAdwaita::mediaControlsBase64StringForIconNameAndType): (WebCore::RenderThemeAdwaita::mediaControlsFormattedStringForDuration): (WebCore::RenderThemeAdwaita::extraMediaControlsStyleSheet): Deleted. * Source/WebCore/rendering/RenderThemeAdwaita.h: * Source/WebKit/ModernMediaControlsGResources.cmake: Added. * Source/WebKit/PlatformGTK.cmake: * Source/WebKit/PlatformWPE.cmake: * Source/cmake/OptionsGTK.cmake: * Source/cmake/OptionsWPE.cmake: * Source/cmake/WebKitFeatures.cmake: * Tools/glib/generate-modern-media-controls-gresource-manifest.py: Added. (get_filenames): Canonical link: https://commits.webkit.org/251219@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295128 268f45cc-cd09-0410-ab3c-d52691b4dbfc
5cf45f5
to
5c4b162
Compare
Committed r295128 (251219@main): https://commits.webkit.org/251219@main Reviewed commits have been landed. Closing PR #1147 and removing active labels. |
5c4b162