Support text track selection in video controls

third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp

Index: third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
diff --git a/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp b/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
index 5b76bf6d3707991fe1b5bf843422099fec2b5876..8cebe2b53b3e355630e18c143f1923853ea48468 100644
--- a/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
+++ b/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
@@ -32,18 +32,25 @@
 #include "bindings/core/v8/ExceptionStatePlaceholder.h"
 #include "core/InputTypeNames.h"
 #include "core/dom/ClientRect.h"
+#include "core/dom/Document.h"
+#include "core/dom/Text.h"
 #include "core/dom/shadow/ShadowRoot.h"
 #include "core/events/MouseEvent.h"
 #include "core/frame/LocalFrame.h"
+#include "core/html/HTMLLabelElement.h"
+#include "core/html/HTMLSpanElement.h"
 #include "core/html/HTMLVideoElement.h"
 #include "core/html/TimeRanges.h"
 #include "core/html/shadow/MediaControls.h"
+#include "core/html/track/TextTrackList.h"
 #include "core/input/EventHandler.h"
 #include "core/layout/LayoutSlider.h"
 #include "core/layout/LayoutTheme.h"
 #include "core/layout/LayoutVideo.h"
 #include "platform/Histogram.h"
 #include "platform/RuntimeEnabledFeatures.h"
+#include "platform/text/PlatformLocale.h"
+#include "public/platform/Platform.h"
 
 namespace blink {
 
@@ -54,6 +61,16 @@ namespace {
 // This is the duration from mediaControls.css
 const double fadeOutDuration = 0.3;
 
+static const QualifiedName& trackIndexAttrName()

[philipj_slow, 2016/03/01 13:01:16]

This is inside an anonymous namespace, so the static keyword isn't needed,
neither here nor for trackIndexOffValue.

[srivats, 2016/03/30 00:46:43]

Done.

+{
+    // Save the track index in an attribute to avoid holding a pointer to the text track element.

[philipj_slow, 2016/03/01 13:01:16]

Remove "element"? Presumably the alternative would be a TextTrack* pointer, and
that's not an element.

[srivats, 2016/03/30 00:46:43]

Done.

+    DEFINE_STATIC_LOCAL(QualifiedName, trackIndexAttr, (nullAtom, "data-track-index", nullAtom));
+    return trackIndexAttr;
+}
+
+// When specified as trackIndex, disable text tracks.
+static const int trackIndexOffValue = -1;
+
 bool isUserInteractionEvent(Event* event)
 {
     const AtomicString& type = event->type();
@@ -353,14 +370,12 @@ void MediaControlToggleClosedCaptionsButtonElement::updateDisplayType()
 {
     bool captionsVisible = mediaElement().closedCaptionsVisible();
     setDisplayType(captionsVisible ? MediaHideClosedCaptionsButton : MediaShowClosedCaptionsButton);
-    setChecked(captionsVisible);
 }
 
 void MediaControlToggleClosedCaptionsButtonElement::defaultEventHandler(Event* event)
 {
     if (event->type() == EventTypeNames::click) {
-        mediaElement().setClosedCaptionsVisible(!mediaElement().closedCaptionsVisible());

[philipj_slow, 2016/03/01 08:51:40]

This was the only call site, can you remove
HTMLMediaElement::setClosedCaptionsVisible entirely?

[srivats, 2016/03/30 00:46:43]

Done.

-        setChecked(mediaElement().closedCaptionsVisible());
+        mediaControls().toggleTextTrackList();
         updateDisplayType();
         event->setDefaultHandled();
     }
@@ -370,6 +385,151 @@ void MediaControlToggleClosedCaptionsButtonElement::defaultEventHandler(Event* e
 
 // ----------------------------
 
+MediaControlTextTrackListElement::MediaControlTextTrackListElement(MediaControls& mediaControls)
+    : MediaControlDivElement(mediaControls, MediaTextTrackList)
+{
+}
+
+PassRefPtrWillBeRawPtr<MediaControlTextTrackListElement> MediaControlTextTrackListElement::create(MediaControls& mediaControls)
+{
+    RefPtrWillBeRawPtr<MediaControlTextTrackListElement> element =
+        adoptRefWillBeNoop(new MediaControlTextTrackListElement(mediaControls));
+    element->setShadowPseudoId(AtomicString("-internal-media-controls-text-track-list",
+        AtomicString::ConstructFromLiteral));
+    element->setIsWanted(false);
+    return element.release();
+}
+
+void MediaControlTextTrackListElement::defaultEventHandler(Event* event)
+{
+    if (event->type() == EventTypeNames::change) {
+        // Identify which input element was selected and set track to showing
+        Node* target = event->target()->toNode();
+        if (!target || !target->isElementNode())
+            return;
+
+        int trackIndex = toElement(target)->getIntegralAttribute(trackIndexAttrName());

[philipj_slow, 2016/03/01 13:01:16]

Nit: Can you switch the order of this and the following line?

[srivats, 2016/03/30 00:46:43]

Done.

+        disableShowingTextTracks();
+        if (trackIndex != trackIndexOffValue) {
+            showTextTrackAtIndex(trackIndex);

[philipj_slow, 2016/03/01 13:01:16]

trackIndex is int but this takes unsigned. ASSERT(trackIndex >= 0).

[srivats, 2016/03/30 00:46:43]

Done.

+            mediaElement().disableAutomaticTextTrackSelection();

[philipj_slow, 2016/03/01 13:01:16]

If a script implements its own track selection menu, it should ideally also
disable automatic text track selection. Did you try to just disable automatic
selection in HTMLMediaElement::textTrackModeChanged? Does it break something
else?

[srivats, 2016/03/30 00:46:43]

I tried it and it seems to be working with some manual testing but timing out
these 3 layout tests:

  media/track/track-kind-user-preference.html [ Timeout ]
  media/track/track-language-preference.html [ Timeout ]
  media/track/track-mode-not-changed-by-new-track.html [ Timeout ]

trackModeChanged also runs when mode changes from hidden to disabled right? So I
guess that would disable auto selection incorrectly.

+        }
+
+        mediaControls().toggleTextTrackList();
+        event->setDefaultHandled();
+    }
+    MediaControlDivElement::defaultEventHandler(event);
+}
+
+void MediaControlTextTrackListElement::changeVisibility(bool isHidden)
+{
+    if (isHidden) {
+        setIsWanted(true);
+        refreshTextTrackListMenu();
+    } else {
+        setIsWanted(false);
+    }
+}
+
+void MediaControlTextTrackListElement::showTextTrackAtIndex(unsigned indexToEnable)
+{
+    TextTrackList* trackList = mediaElement().textTracks();
+    if (trackList->length() == 0)

[philipj_slow, 2016/03/01 13:01:16]

TextTrackList::anonymousIndexedGetter already handles out-of-bounds by returning
nullptr, I suggest removing these checks and relying on that, which would also
make it obvious that track is never dereferenced if null.

[srivats, 2016/03/30 00:46:43]

Done.

+        return;
+    if (indexToEnable >= trackList->length())
+        return;
+    TextTrack* track = trackList->anonymousIndexedGetter(indexToEnable);
+    if (track->canBeRendered())
+        track->setMode(TextTrack::showingKeyword());
+}
+
+void MediaControlTextTrackListElement::disableShowingTextTracks()
+{
+    TextTrackList* trackList = mediaElement().textTracks();
+    for (unsigned i = 0; i < trackList->length(); ++i) {
+        TextTrack* track = trackList->anonymousIndexedGetter(i);
+        if (track->mode() == TextTrack::showingKeyword())
+            track->setMode(TextTrack::disabledKeyword());
+    }
+}
+
+String MediaControlTextTrackListElement::getTextTrackLabel(TextTrack* track)
+{
+    if (!track)
+        return mediaElement().locale().queryString(WebLocalizedString::TextTracksOff);
+
+    String trackLabel = track->label();
+
+    if (trackLabel.isEmpty())
+        trackLabel = track->language();

[philipj_slow, 2016/03/01 13:01:16]

This was discussed in
https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
with no resolution.

To make it abundantly clear that the srclang attribute is only for matching the
user language and not part of the UI, I suggest simply not falling back to it,
so that if no label is given it will show as "Unknown".

[srivats, 2016/03/30 00:46:43]

Apologies for that. I tried to respond back to as many comments as possible on
that code review.
Removed fallback to srclang like you suggested.

+
+    if (trackLabel.isEmpty())
+        trackLabel = String(mediaElement().locale().queryString(WebLocalizedString::TextTracksNoLabel));
+
+    return trackLabel;
+}
+
+// TextTrack parameter when passed in as a nullptr, creates the "Off" list item in the track list.
+PassRefPtrWillBeRawPtr<Element> MediaControlTextTrackListElement::createTextTrackListItem(TextTrack* track)
+{
+    Document& document = this->document();
+    int trackIndex = track ? track->trackIndex() : trackIndexOffValue;
+    RefPtrWillBeRawPtr<HTMLLabelElement> trackItem = HTMLLabelElement::create(document, 0);

[philipj_slow, 2016/03/01 13:01:16]

s/0/nullptr/

[srivats, 2016/03/30 00:46:43]

Done.

+    trackItem->setShadowPseudoId(AtomicString("-internal-media-controls-text-track-list-item",
+        AtomicString::ConstructFromLiteral));
+    RefPtrWillBeRawPtr<HTMLInputElement> trackItemInput = HTMLInputElement::create(document, 0, false);

[philipj_slow, 2016/03/01 13:01:16]

s/0/nullptr/

[srivats, 2016/03/30 00:46:43]

Done.

+    trackItemInput->setShadowPseudoId(AtomicString("-internal-media-controls-text-track-list-item-input",
+        AtomicString::ConstructFromLiteral));
+    trackItemInput->setType(InputTypeNames::radio);

[philipj_slow, 2016/03/01 13:01:16]

Radio seems semantically accurate in the normal case, but it is possible for
multiple of these to be enabled at the same time. They don't belong to the same
radio button group it seems, so that's no problem in practice it seems. Still,
can you make these checkboxes instead? They're even rendered with a check mark
:)

[srivats, 2016/03/30 00:46:43]

I started off with radio in a radio group but eventually ended up realizing
radio group is not going to work for multiple selected tracks and got rid of the
radio group.
Changed it to a checkbox but retained my check mark :)

+    trackItemInput->setIntegralAttribute(trackIndexAttrName(), trackIndex);
+    if (!mediaElement().closedCaptionsVisible()) {

[philipj_slow, 2016/03/01 13:01:16]

This is now not terribly accurate, it's something more like textTracksVisible()
we' be interested in.  To make this all a bit more obvious, can you handle it in
MediaControlTextTrackListElement::refreshTextTrackListMenu by checking if any of
the tracks actually added are visible, and if not set the "Off" option as the
selected one? Simply creating this item last and inserting it before the other
tracks ought to do it.

[srivats, 2016/03/30 00:46:43]

Renamed closedCaptionsVisible to textTracksVisible.
Addressing the second bit is not going to be trivial since setChecked is set on
the input element of the track item and has to be done in the context of where
the individual trackItem elements are created.

+        if (!track)
+            trackItemInput->setChecked(true);
+    } else {
+        // If there are multiple text tracks set to showing, they must all have
+        // checkmarks displayed.
+        if (track && track->mode() == TextTrack::showingKeyword())
+            trackItemInput->setChecked(true);
+    }
+    RefPtrWillBeRawPtr<HTMLSpanElement> trackKindMarker = HTMLSpanElement::create(document);
+    if (track && track->kind() == track->captionsKeyword()) {
+        trackKindMarker->setShadowPseudoId(AtomicString("-internal-media-controls-text-track-list-kind-captions",
+            AtomicString::ConstructFromLiteral));
+    } else if (track && track->kind() == track->subtitlesKeyword()) {

[philipj_slow, 2016/03/01 13:01:16]

Can you use an else branch and ASSERT(track->kind() ==
track->subtitlesKeyword()) to not give the impression that there are other
possibilities here? (Only makes sense if you wrap both in if (track), so that's
what I mean.)

[srivats, 2016/03/30 00:46:43]

Done.

+        trackKindMarker->setShadowPseudoId(AtomicString("-internal-media-controls-text-track-list-kind-subtitles",
+            AtomicString::ConstructFromLiteral));
+    }
+    trackItem->appendChild(trackItemInput);
+    trackItem->appendChild(Text::create(document, getTextTrackLabel(track)));
+    trackItem->appendChild(trackKindMarker);
+    return trackItem;
+}
+
+void MediaControlTextTrackListElement::refreshTextTrackListMenu()
+{
+    if (!mediaElement().hasClosedCaptions() || !mediaElement().textTracksAreReady())
+        return;
+
+    removeChildren(OmitSubtreeModifiedEvent);
+
+    // Construct a menu for subtitles and captions
+    RefPtrWillBeRawPtr<HTMLDivElement> tracksSection = HTMLDivElement::create(this->document());

[philipj_slow, 2016/03/01 13:01:16]

This div element doesn't seem to be included in the "The media controls DOM
structure looks like" documentation, can you add it?

Also looks like you can just use document() here, there's no other document
variable to disambiguate from.

[srivats, 2016/03/30 00:46:43]

Since we don't need a div element that holds both header and track list items
anymore, I got rid of the section entirely.

+    tracksSection->setAttribute(roleAttr, radiogroupAttr.localName());

[philipj_slow, 2016/03/01 13:01:16]

Does this change how this looks to a11y tools, and are there any tests for it?

[srivats, 2016/03/30 00:46:43]

Got rid of the radio and hence this too

+    // Pass in a nullptr to createTextTrackListItem to create the "Off" track item.
+    tracksSection->appendChild(createTextTrackListItem(nullptr));
+
+    TextTrackList* trackList = mediaElement().textTracks();
+    for (unsigned i = 0; i < trackList->length(); i++) {
+        TextTrack* track = trackList->anonymousIndexedGetter(i);
+        if (!track->canBeRendered())
+            continue;
+        tracksSection->appendChild(createTextTrackListItem(track));
+    }
+    appendChild(tracksSection);
+}
+
+// ----------------------------
+
 MediaControlTimelineElement::MediaControlTimelineElement(MediaControls& mediaControls)
     : MediaControlInputElement(mediaControls, MediaSlider)
 {