Bug 123097 - [GTK] MEDIA_CONTROLS_SCRIPT support
Summary: [GTK] MEDIA_CONTROLS_SCRIPT support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL:
Keywords:
Depends on: 120895
Blocks: 126301
  Show dependency treegraph
 
Reported: 2013-10-21 08:47 PDT by Philippe Normand
Modified: 2014-02-14 15:55 PST (History)
29 users (show)

See Also:


Attachments
Patch (571.96 KB, patch)
2014-01-30 11:40 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (572.35 KB, patch)
2014-02-01 17:09 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (573.17 KB, patch)
2014-02-03 11:12 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (573.34 KB, patch)
2014-02-12 04:52 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2013-10-21 08:47:15 PDT
Bug 120895 added support for js/css media controls definitions. We should try to adopt this feature as well.
Comment 1 Philippe Normand 2013-12-12 01:35:06 PST
Why should this bug depend on #120895 ?
Comment 2 Xabier Rodríguez Calvar 2013-12-12 02:06:35 PST
(In reply to comment #1)
> Why should this bug depend on #120895 ?

Because we use that code and for bug tracking.
Comment 3 Philippe Normand 2013-12-12 02:10:24 PST
(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 :)
Comment 4 Xabier Rodríguez Calvar 2013-12-26 05:49:11 PST
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?
Comment 5 Xabier Rodríguez Calvar 2014-01-30 11:40:18 PST
Created attachment 222701 [details]
Patch
Comment 6 Martin Robinson 2014-01-30 16:41:22 PST
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.
Comment 7 Xabier Rodríguez Calvar 2014-02-01 17:09:50 PST
Created attachment 222893 [details]
Patch
Comment 8 Xabier Rodríguez Calvar 2014-02-01 17:23:59 PST
(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.
Comment 9 Jer Noble 2014-02-02 07:53:31 PST
I'll take a look at this on Monday (tomorrow).
Comment 10 Martin Robinson 2014-02-03 10:05:22 PST
(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.
Comment 11 Xabier Rodríguez Calvar 2014-02-03 11:12:50 PST
Created attachment 223000 [details]
Patch
Comment 12 Xabier Rodríguez Calvar 2014-02-03 11:13:46 PST
(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.
Comment 13 Xabier Rodríguez Calvar 2014-02-05 09:57:58 PST
(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!
Comment 14 Jer Noble 2014-02-05 10:03:48 PST
(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 15 Jer Noble 2014-02-05 10:15:28 PST
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.
Comment 16 Xabier Rodríguez Calvar 2014-02-05 10:28:43 PST
(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.
Comment 17 Jer Noble 2014-02-05 10:31:27 PST
(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?
Comment 18 Xabier Rodríguez Calvar 2014-02-12 04:52:11 PST
Created attachment 223961 [details]
Patch
Comment 19 Xabier Rodríguez Calvar 2014-02-12 04:56:08 PST
(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.
Comment 20 Xabier Rodríguez Calvar 2014-02-12 07:29:27 PST
In case it is not clear, I would like to have an explicit r+, if possible :)
Comment 21 Jer Noble 2014-02-12 08:13:54 PST
Comment on attachment 223961 [details]
Patch

r=me. Thanks!
Comment 22 James Craig 2014-02-12 08:56:05 PST
I don't think this is ready yet. In the process of adding more comments.
Comment 23 James Craig 2014-02-12 09:25:49 PST
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
Comment 24 James Craig 2014-02-12 09:27:44 PST
(In reply to comment #23)

> > Source/WebCore/css/mediaControlsGtk.css:197
> > +    overflow: hidden;
> 
> ditto

Ignore that one. Scanned too quickly.
Comment 25 James Craig 2014-02-12 09:30:12 PST
I would request final review from Joanie (:joanie) and/or Mario (:msanchez) for the Orca/FKA implications.
Comment 26 Mario Sanchez Prada 2014-02-12 09:51:14 PST
(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
Comment 27 James Craig 2014-02-12 10:40:48 PST
Okay. Commit away. The other nits (hiding visible focus, CSS redundancy) can be fixed in follow-up bugs.
Comment 28 James Craig 2014-02-12 10:43:59 PST
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 29 Xabier Rodríguez Calvar 2014-02-13 02:54:06 PST
Comment on attachment 223961 [details]
Patch

As per comment 27, committing
Comment 30 WebKit Commit Bot 2014-02-13 03:23:33 PST
Comment on attachment 223961 [details]
Patch

Clearing flags on attachment: 223961

Committed r164024: <http://trac.webkit.org/changeset/164024>
Comment 31 WebKit Commit Bot 2014-02-13 03:23:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 32 Brendan Long 2014-02-14 14:19:29 PST
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.
Comment 33 Xabier Rodríguez Calvar 2014-02-14 14:50:34 PST
(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
Comment 34 Xabier Rodríguez Calvar 2014-02-14 14:56:03 PST
(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
Comment 35 Brendan Long 2014-02-14 15:55:21 PST
(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.