Bug 123097 – [GTK] MEDIA_CONTROLS_SCRIPT support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

Bug 120895 added support for js/css media controls definitions. We should try to adopt this feature as well.

[Philippe Normand]

Why should this bug depend on #120895 ?

[Xabier Rodríguez Calvar]

(In reply to comment #1)
> Why should this bug depend on #120895 ?

Because we use that code and for bug tracking.

[Philippe Normand]

(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 :)

[Xabier Rodríguez Calvar]

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?

[Xabier Rodríguez Calvar]

Created attachment 222701 [details]
Patch

[Martin Robinson]

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.

[Xabier Rodríguez Calvar]

Created attachment 222893 [details]
Patch

[Xabier Rodríguez Calvar]

(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.

[Jer Noble]

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

[Martin Robinson]

(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.

[Xabier Rodríguez Calvar]

Created attachment 223000 [details]
Patch

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

(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!

[Jer Noble]

(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. :)

[Jer Noble]

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.

[Xabier Rodríguez Calvar]

(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.

[Jer Noble]

(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?

[Xabier Rodríguez Calvar]

Created attachment 223961 [details]
Patch

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

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

[Jer Noble]

Comment on attachment 223961 [details]
Patch

r=me. Thanks!

[James Craig]

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

[James Craig]

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

[James Craig]

(In reply to comment #23)

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

Ignore that one. Scanned too quickly.

[James Craig]

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

[Mario Sanchez Prada]

(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

[James Craig]

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

[James Craig]

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.

[Xabier Rodríguez Calvar]

Comment on attachment 223961 [details]
Patch

As per comment 27, committing

[WebKit Commit Bot]

Comment on attachment 223961 [details]
Patch

Clearing flags on attachment: 223961

Committed r164024: <http://trac.webkit.org/changeset/164024>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brendan Long]

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.

[Xabier Rodríguez Calvar]

(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

[Xabier Rodríguez Calvar]

(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

[Brendan Long]

(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.