Bug 123097

Why should this bug depend on #120895 ?

(In reply to comment #1) > Why should this bug depend on #120895 ? Because we use that code and for bug tracking.

(In reply to comment #2) > (In reply to comment #1) > > Why should this bug depend on #120895 ? > > Because we use that code and for bug tracking. If you need to link on all the bugs containing code you depend on then you should add more to the list :)

Jon, as part of this bug I am fixing the captions button and it seems that the one bundled in CSS is the user-invisible-symbolic one. For now I am using that and the stock fallback will be GTK_STOCK_JUSTIFY_FILL. We still need the fallback and I think using an icon for its current shape is not a good idea as the icon could be reshaped in next versions, which would give a completely different impression. I think we should file a bug for the symbolic icons to get a new captions or subtitles one and I also expect from you a recommendation for the stock fallback. Any comments?

Created attachment 222701 [details] Patch

Comment on attachment 222701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222701&action=review Nice. I haven't had a chance to look closely at the new code, but here are some first-pass observations. A person from Apple will need to review the changes to the Mac script. Perhaps it's better if we fork it for GTK+, if changes are necessary. The volume button appears to be missing in LayoutTests/platform/gtk/http/tests/media/video-buffered-range-contains-currentTime-expected.png. > Source/WebCore/CMakeLists.txt:3154 > +#Generate media controls with javascript Please remove this comment. > Source/WebCore/CMakeLists.txt:3155 > +if ("${WebCore_USER_AGENT_SCRIPTS}" STRLESS "") This can just be "if (WebCore_USER_AGENT_SCRIPTS)" > Source/WebCore/CMakeLists.txt:3156 > + # Needed variables: Needed -> Necessary > Source/WebCore/CMakeLists.txt:3157 > + # WebCore_USER_AGENT_SCRIPTS containing the javacript sources list javascript -> JavaScript > Source/WebCore/CMakeLists.txt:3159 > + # WebCore_USER_AGENT_SCRIPTS_DEPENDENCIES containing the source file that will load the scripts to add the proper > + # dependency and having them built at the right moment This seems unused? > Source/WebCore/CMakeLists.txt:3169 > + list(APPEND WebCore_SOURCES > + Modules/mediacontrols/MediaControlsHost.cpp > + ) > + list(APPEND WebCore_IDL_FILES > + Modules/mediacontrols/MediaControlsHost.idl > + ) > + list(APPEND WebCore_INCLUDE_DIRECTORIES > + "${WEBCORE_DIR}/Modules/mediacontrols/" > + ) Just add these to the main lists. > Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:164 > + // Caption menu has to be centered to the caption button Please make sure your comments end with periods. > Source/WebCore/PlatformGTK.cmake:469 > +set(WebCore_USER_AGENT_SCRIPTS > + ${WEBCORE_DIR}/Modules/mediacontrols/mediaControlsApple.js > + ${WEBCORE_DIR}/Modules/mediacontrols/mediaControlsGtk.js > +) > +set(WebCore_USER_AGENT_SCRIPTS_DEPENDENCIES ${WEBCORE_DIR}/platform/gtk/RenderThemeGtk.cpp) > + This should really be defined right after WebCore_INCLUDE_DIRECTORIES, where the other shared variables are defined. > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:542 > + // Check for the button to be in paused state to display the play icon > + bool play = nodeHasClass(node, "paused"); You can remove the need for the comment through better variable naming: play -> showPlayButton > LayoutTests/platform/gtk-wk2/TestExpectations:465 > +Bug(GTK) media/video-volume-slider.html [ ImageOnlyFailure ] What's wrong here? > LayoutTests/platform/gtk/accessibility/media-element-expected.txt:17 > - description: AXDescription: video playback > - role: AXRole: AXToolbar > + description: AXDescription: Play > + role: AXRole: AXButton Can't we preserve these test results through ARIA roles? Joanie might be able to help here.

Created attachment 222893 [details] Patch

(In reply to comment #6) > Nice. I haven't had a chance to look closely at the new code, but here are some first-pass observations. A person from Apple will need to review the changes to the Mac script. Perhaps it's better if we fork it for GTK+, if changes are necessary. Of course it is necessary that somebody from Apple has a look at the changes, though there are not important changes in the code, but some refactorings to improve readability and a new class. Forking them would be a solution but I think it is better to share the code than keeping things apart. > The volume button appears to be missing in LayoutTests/platform/gtk/http/tests/media/video-buffered-range-contains-currentTime-expected.png. It dissapeared because it is not shown now as that tests seems to not have any audio stream and as it doens't have audio streams, the controls don't show the mute button. > > Source/WebCore/CMakeLists.txt:3154 > > +#Generate media controls with javascript > > Please remove this comment. Done. > > Source/WebCore/CMakeLists.txt:3155 > > +if ("${WebCore_USER_AGENT_SCRIPTS}" STRLESS "") > > This can just be "if (WebCore_USER_AGENT_SCRIPTS)" Done. > > Source/WebCore/CMakeLists.txt:3156 > > + # Needed variables: > > Needed -> Necessary Done. > > Source/WebCore/CMakeLists.txt:3157 > > + # WebCore_USER_AGENT_SCRIPTS containing the javacript sources list > > javascript -> JavaScript Done. > > Source/WebCore/CMakeLists.txt:3159 > > + # WebCore_USER_AGENT_SCRIPTS_DEPENDENCIES containing the source file that will load the scripts to add the proper > > + # dependency and having them built at the right moment > > This seems unused? What I assumed is that the _DEPENDENCIES was a "magic" variable that adds that as dependencies for that target. At least it compiles as expected. > > Source/WebCore/CMakeLists.txt:3169 > > + list(APPEND WebCore_SOURCES > > + Modules/mediacontrols/MediaControlsHost.cpp > > + ) > > + list(APPEND WebCore_IDL_FILES > > + Modules/mediacontrols/MediaControlsHost.idl > > + ) > > + list(APPEND WebCore_INCLUDE_DIRECTORIES > > + "${WEBCORE_DIR}/Modules/mediacontrols/" > > + ) > > Just add these to the main lists. I think it is not a good idea as this things should only be built when VIDEO is enabled. I kept it as it is now. > > Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:164 > > + // Caption menu has to be centered to the caption button > > Please make sure your comments end with periods. Done. > > Source/WebCore/PlatformGTK.cmake:469 > > +set(WebCore_USER_AGENT_SCRIPTS > > + ${WEBCORE_DIR}/Modules/mediacontrols/mediaControlsApple.js > > + ${WEBCORE_DIR}/Modules/mediacontrols/mediaControlsGtk.js > > +) > > +set(WebCore_USER_AGENT_SCRIPTS_DEPENDENCIES ${WEBCORE_DIR}/platform/gtk/RenderThemeGtk.cpp) > > + > > This should really be defined right after WebCore_INCLUDE_DIRECTORIES, where the other shared variables are defined. Instead of moving this where you suggest, I preferred to move it where the USER_AGENT_STYLE_SHEETS is defined. > > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:542 > > + // Check for the button to be in paused state to display the play icon > > + bool play = nodeHasClass(node, "paused"); > > You can remove the need for the comment through better variable naming: play -> showPlayButton Done. > > LayoutTests/platform/gtk-wk2/TestExpectations:465 > > +Bug(GTK) media/video-volume-slider.html [ ImageOnlyFailure ] > > What's wrong here? There is some difference in the way the things are loaded (or at least reported as loaded or not) between WK1 and WK2 so the timeline is filled in one case and not in the other. This is not a problem of this patch and it was happening already with the former code, so I didn't touch it, but flagged the test. > > LayoutTests/platform/gtk/accessibility/media-element-expected.txt:17 > > - description: AXDescription: video playback > > - role: AXRole: AXToolbar > > + description: AXDescription: Play > > + role: AXRole: AXButton > > Can't we preserve these test results through ARIA roles? Joanie might be able to help here. The roles change because the elements change and according to Mario, who I commented the situation with and CCed in the bug, the rebaseline is ok.

I'll take a look at this on Monday (tomorrow).

(In reply to comment #8) > (In reply to comment #6) > > Nice. I haven't had a chance to look closely at the new code, but here are some first-pass observations. A person from Apple will need to review the changes to the Mac script. Perhaps it's better if we fork it for GTK+, if changes are necessary. > > Of course it is necessary that somebody from Apple has a look at the changes, though there are not important changes in the code, but some refactorings to improve readability and a new class. Forking them would be a solution but I think it is better to share the code than keeping things apart. Makes sense. > > > Source/WebCore/CMakeLists.txt:3159 > > > + # WebCore_USER_AGENT_SCRIPTS_DEPENDENCIES containing the source file that will load the scripts to add the proper > > > + # dependency and having them built at the right moment > > > > This seems unused? > > What I assumed is that the _DEPENDENCIES was a "magic" variable that adds that as dependencies for that target. At least it compiles as expected. Unlike autotools, there are no magic variables in CMake. Either remove it or use it. :) > > > Source/WebCore/CMakeLists.txt:3169 > > > + list(APPEND WebCore_SOURCES > > > + Modules/mediacontrols/MediaControlsHost.cpp > > > + ) > > > + list(APPEND WebCore_IDL_FILES > > > + Modules/mediacontrols/MediaControlsHost.idl > > > + ) > > > + list(APPEND WebCore_INCLUDE_DIRECTORIES > > > + "${WEBCORE_DIR}/Modules/mediacontrols/" > > > + ) > > > > Just add these to the main lists. > > I think it is not a good idea as this things should only be built when VIDEO is enabled. I kept it as it is now. That's not how the CMake build works (there are a couple exceptions, but they are exceptions). Files that use VIDEO are on the main lists as well. Everything goes on the main list and conditional compilation is used to control whether it is in the final binary. I can see that MediaControlsHost.cpp has the proper guard in place. Please move it to the main list. > > This should really be defined right after WebCore_INCLUDE_DIRECTORIES, where the other shared variables are defined. > > Instead of moving this where you suggest, I preferred to move it where the USER_AGENT_STYLE_SHEETS is defined. I agree that's a much better spot. > > > LayoutTests/platform/gtk/accessibility/media-element-expected.txt:17 > > > - description: AXDescription: video playback > > > - role: AXRole: AXToolbar > > > + description: AXDescription: Play > > > + role: AXRole: AXButton > > > > Can't we preserve these test results through ARIA roles? Joanie might be able to help here. > > The roles change because the elements change and according to Mario, who I commented the situation with and CCed in the bug, the rebaseline is ok. Okay.

Created attachment 223000 [details] Patch

(In reply to comment #10) > > > > Source/WebCore/CMakeLists.txt:3159 > > > > + # WebCore_USER_AGENT_SCRIPTS_DEPENDENCIES containing the source file that will load the scripts to add the proper > > > > + # dependency and having them built at the right moment > > > > > > This seems unused? > > > > What I assumed is that the _DEPENDENCIES was a "magic" variable that adds that as dependencies for that target. At least it compiles as expected. > > Unlike autotools, there are no magic variables in CMake. Either remove it or use it. :) Done. > > > > Source/WebCore/CMakeLists.txt:3169 > > > > + list(APPEND WebCore_SOURCES > > > > + Modules/mediacontrols/MediaControlsHost.cpp > > > > + ) > > > > + list(APPEND WebCore_IDL_FILES > > > > + Modules/mediacontrols/MediaControlsHost.idl > > > > + ) > > > > + list(APPEND WebCore_INCLUDE_DIRECTORIES > > > > + "${WEBCORE_DIR}/Modules/mediacontrols/" > > > > + ) > > > > > > Just add these to the main lists. > > > > I think it is not a good idea as this things should only be built when VIDEO is enabled. I kept it as it is now. > > That's not how the CMake build works (there are a couple exceptions, but they are exceptions). Files that use VIDEO are on the main lists as well. Everything goes on the main list and conditional compilation is used to control whether it is in the final binary. I can see that MediaControlsHost.cpp has the proper guard in place. Please move it to the main list. Done.

(In reply to comment #9) > I'll take a look at this on Monday (tomorrow). I tried to contact you on IRC, but I couldn't find you. Would it be possible to get this reviewed? Thanks a lot in advance!

(In reply to comment #13) > (In reply to comment #9) > > I'll take a look at this on Monday (tomorrow). > > I tried to contact you on IRC, but I couldn't find you. Would it be possible to get this reviewed? > > Thanks a lot in advance! Sure thing, I'll look at it this morning. The Apple.js changes look fine, but I suspect you'd like a full review. :)

Comment on attachment 223000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223000&action=review > Source/WebCore/CMakeLists.txt:3170 > + add_custom_command( > + OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/UserAgentScriptsData.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/UserAgentScripts.h > + MAIN_DEPENDENCY ${WEBCORE_DIR}/css/make-css-file-arrays.pl > + DEPENDS ${WebCore_USER_AGENT_SCRIPTS} ${WEBCORE_DIR}/bindings/scripts/preprocessor.pm > + COMMAND ${PERL_EXECUTABLE} -I${WEBCORE_DIR}/bindings/scripts ${WEBCORE_DIR}/css/make-css-file-arrays.pl --defines "${FEATURE_DEFINES_WITH_SPACE_SEPARATOR}" --preprocessor "${CODE_GENERATOR_PREPROCESSOR}" ${DERIVED_SOURCES_WEBCORE_DIR}/UserAgentScripts.h ${DERIVED_SOURCES_WEBCORE_DIR}/UserAgentScriptsData.cpp ${WebCore_USER_AGENT_SCRIPTS} > + VERBATIM) FYI: I'm working on a patch which would use JSMin rather than the clang/gcc preprocessor for JavaScript files. See bug #127559. > Source/WebCore/GNUmakefile.am:273 > +USER_AGENT_SCRIPT_FILES = $(WebCore)/Modules/mediacontrols/mediaControlsApple.js $(WebCore)/Modules/mediacontrols/mediaControlsGtk.js > +DerivedSources/WebCore/UserAgentScriptsData.cpp: DerivedSources/WebCore/UserAgentScripts.h > +DerivedSources/WebCore/UserAgentScripts.h: $(WebCore)/css/make-css-file-arrays.pl $(WebCore)/bindings/scripts/preprocessor.pm $(USER_AGENT_SCRIPT_FILES) WebKitFeatures.txt > + $(AM_V_at)$(PERL) -I$(WebCore)/bindings/scripts $< --defines "$(feature_defines)" $@ DerivedSources/WebCore/UserAgentScriptsData.cpp $(USER_AGENT_SCRIPT_FILES) Ditto. r=me, with a couple of nits: > Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:27 > + inheritFrom: function(parent) { > + for (var property in parent) { > + if (!this.hasOwnProperty(property)) > + this[property] = parent[property]; > + } > + }, Since this method is used in two subclasses of Controller, we should consider moving this method into the base class. But that work can be done in another bug. > LayoutTests/media/click-volume-bar-not-pausing.html:54 > + setTimeout(function() { > + // Forcing relayout calculations to say that you are > + // triggering the volume slider to show up for > + // controls that work that way. > + document.body.offsetTop; > + > + volumeSliderCoordinates = calculateElementCoordinates("volume-slider"); > + eventSender.mouseMoveTo(volumeSliderCoordinates[0], volumeSliderCoordinates[1]); > + > + eventSender.mouseDown(); > + eventSender.mouseUp(); > + }, 130); Why 130ms? Is it because of your fade animation? An opacity animation should affect the volume slider clickability. > LayoutTests/media/media-volume-slider-rendered-normal.html:62 > + }, 130); Ditto, w.r.t. 130ms.

(In reply to comment #15) I'll have a look at the rest. > > LayoutTests/media/click-volume-bar-not-pausing.html:54 > > + setTimeout(function() { > > + // Forcing relayout calculations to say that you are > > + // triggering the volume slider to show up for > > + // controls that work that way. > > + document.body.offsetTop; > > + > > + volumeSliderCoordinates = calculateElementCoordinates("volume-slider"); > > + eventSender.mouseMoveTo(volumeSliderCoordinates[0], volumeSliderCoordinates[1]); > > + > > + eventSender.mouseDown(); > > + eventSender.mouseUp(); > > + }, 130); > > Why 130ms? Is it because of your fade animation? An opacity animation should affect the volume slider clickability. > > > LayoutTests/media/media-volume-slider-rendered-normal.html:62 > > + }, 130); > > Ditto, w.r.t. 130ms. It's not an opacity but a height animation and clickability is affected, therefore, the slider can't be found.

(In reply to comment #16) > (In reply to comment #15) > > I'll have a look at the rest. > > > > LayoutTests/media/click-volume-bar-not-pausing.html:54 > > > + setTimeout(function() { > > > + // Forcing relayout calculations to say that you are > > > + // triggering the volume slider to show up for > > > + // controls that work that way. > > > + document.body.offsetTop; > > > + > > > + volumeSliderCoordinates = calculateElementCoordinates("volume-slider"); > > > + eventSender.mouseMoveTo(volumeSliderCoordinates[0], volumeSliderCoordinates[1]); > > > + > > > + eventSender.mouseDown(); > > > + eventSender.mouseUp(); > > > + }, 130); > > > > Why 130ms? Is it because of your fade animation? An opacity animation should affect the volume slider clickability. > > > > > LayoutTests/media/media-volume-slider-rendered-normal.html:62 > > > + }, 130); > > > > Ditto, w.r.t. 130ms. > > It's not an opacity but a height animation and clickability is affected, therefore, the slider can't be found. Ah, that's somewhat unfortunate. (It also means tests will break if you ever change the speed of that animation.) Can we add a DRT-only User Agent style sheet which overrides that animation duration?

Created attachment 223961 [details] Patch

(In reply to comment #15) > FYI: I'm working on a patch which would use JSMin rather than the clang/gcc preprocessor for JavaScript files. See bug #127559. As your patch has landed already, I changed this too. > > Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:27 > > + inheritFrom: function(parent) { > > + for (var property in parent) { > > + if (!this.hasOwnProperty(property)) > > + this[property] = parent[property]; > > + } > > + }, > > Since this method is used in two subclasses of Controller, we should consider moving this method into the base class. But that work can be done in another bug. I note it for the new bug. > > LayoutTests/media/click-volume-bar-not-pausing.html:54 > > + setTimeout(function() { > > + // Forcing relayout calculations to say that you are > > + // triggering the volume slider to show up for > > + // controls that work that way. > > + document.body.offsetTop; > > + > > + volumeSliderCoordinates = calculateElementCoordinates("volume-slider"); > > + eventSender.mouseMoveTo(volumeSliderCoordinates[0], volumeSliderCoordinates[1]); > > + > > + eventSender.mouseDown(); > > + eventSender.mouseUp(); > > + }, 130); > > Why 130ms? Is it because of your fade animation? An opacity animation should affect the volume slider clickability. I fixed the tests by using the Internals as we discussed on IRC. It works both in WK1 and WK2.

In case it is not clear, I would like to have an explicit r+, if possible :)

Comment on attachment 223961 [details] Patch r=me. Thanks!

I don't think this is ready yet. In the process of adding more comments.

Comment on attachment 223961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223961&action=review > Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:153 > + handleCaptionButtonClicked: function(event) > + { > + // Handled with mouseover and mouseout. > + }, This appears to break full keyboard access and potentially Orca support. Not sure about Orca, but if this change were in the Apple version, it would prevent anyone but mouse users from getting to this menu, including VoiceOver users, Switch Control users, Full Keyboard Access users, etc. > Source/WebCore/css/mediaControlsGtk.css:180 > +audio::-webkit-media-controls-current-time-display, video::-webkit-media-controls-current-time-display { > + display: block; > +} > + > +audio::-webkit-media-controls-time-remaining-display, > +video::-webkit-media-controls-time-remaining-display { > + display: none; > +} > + > +audio::-webkit-media-controls-time-remaining-display.show, > +video::-webkit-media-controls-time-remaining-display.show { > + display: block; > +} > + > +audio::-webkit-media-controls-current-time-display.hidden, > +video::-webkit-media-controls-current-time-display.hidden, > +audio::-webkit-media-controls-time-remaining-display.hidden, > +video::-webkit-media-controls-time-remaining-display.hidden { > + display: none; > +} > + It's redundant to specify the default state, the shown, state, and the hidden state for all these. It'd be better to just specify the default state and then use the hidden attribute. This will also allow you to remove the general shown/hidden classnames and just set Element.hidden to true or false. Note: Because WebKit doesn't use the !important directive the in the default style sheet rule for @hidden, you need to use it in a new style block for this file, but this is still less redundant and more readable than the current duplications. [hidden] { display: none !important; } > Source/WebCore/css/mediaControlsGtk.css:192 > + outline: none; You need to reenable a visible focus style (could be outline, but does not have to be) when the element becomes focused, and potentially active. See some of the related changes (:focus, :active) coming to the Apple styles in bug 127267. > Source/WebCore/css/mediaControlsGtk.css:197 > + overflow: hidden; ditto > Source/WebCore/css/mediaControlsGtk.css:239 > + outline: none; ditto. back this up with visible :focus/:active styles. > Source/WebCore/css/mediaControlsGtk.css:292 > + outline: none; ditto

(In reply to comment #23) > > Source/WebCore/css/mediaControlsGtk.css:197 > > + overflow: hidden; > > ditto Ignore that one. Scanned too quickly.

I would request final review from Joanie (:joanie) and/or Mario (:msanchez) for the Orca/FKA implications.

(In reply to comment #25) > I would request final review from Joanie (:joanie) and/or Mario (:msanchez) for the Orca/FKA implications. As I already commented to Xabier during FOSDEM, I don't see any problem accessibility-wise here. The reason why the expectations need to be updated is that the hierarchy in which the accessibility hierarchy is built upon has changed, and the result mappings seem sane to me. So no problems from that side, IMHO

Okay. Commit away. The other nits (hiding visible focus, CSS redundancy) can be fixed in follow-up bugs.

You might also want to cc yourself on bug 117857 because the full keyboard access bugs will be more obvious once that change rolls in.

Comment on attachment 223961 [details] Patch As per comment 27, committing

Comment on attachment 223961 [details] Patch Clearing flags on attachment: 223961 Committed r164024: <http://trac.webkit.org/changeset/164024>

All reviewed patches have been landed. Closing bug.

On my machine, this commit causes the subtitle/caption controls to stop working, and the browser to crash if you hit the "Play" button. I'm trying to track down the cause now.

(In reply to comment #32) > On my machine, this commit causes the subtitle/caption controls to stop working, and the browser to crash if you hit the "Play" button. I'm trying to track down the cause now. Are you using a debug build? https://bugs.webkit.org/show_bug.cgi?id=128820

(In reply to comment #33) > (In reply to comment #32) > > On my machine, this commit causes the subtitle/caption controls to stop working, and the browser to crash if you hit the "Play" button. I'm trying to track down the cause now. > > Are you using a debug build? > > https://bugs.webkit.org/show_bug.cgi?id=128820 Btw, the subtitles control doesn't work now with a press, but with a mouse hover to be consistent with the volume bar

(In reply to comment #33) > (In reply to comment #32) > > On my machine, this commit causes the subtitle/caption controls to stop working, and the browser to crash if you hit the "Play" button. I'm trying to track down the cause now. > > Are you using a debug build? > > https://bugs.webkit.org/show_bug.cgi?id=128820 Yeah, that's the one. Thanks.