Support text track selection in video controls

Source/core/html/shadow/MediaControlElements.cpp

Index: Source/core/html/shadow/MediaControlElements.cpp
diff --git a/Source/core/html/shadow/MediaControlElements.cpp b/Source/core/html/shadow/MediaControlElements.cpp
index 3e2c7537ea26df6c70a25bb2dc7c27e768fc543f..fcb9017461725a36924fdf45c15f48810bf057f6 100644
--- a/Source/core/html/shadow/MediaControlElements.cpp
+++ b/Source/core/html/shadow/MediaControlElements.cpp
@@ -33,6 +33,9 @@
 #include "bindings/core/v8/ExceptionStatePlaceholder.h"
 #include "core/InputTypeNames.h"
 #include "core/dom/DOMTokenList.h"
+#include "core/dom/Document.h"
+#include "core/dom/Element.h"
+#include "core/dom/Text.h"
 #include "core/dom/shadow/ShadowRoot.h"
 #include "core/events/MouseEvent.h"
 #include "core/frame/LocalFrame.h"
@@ -40,11 +43,14 @@
 #include "core/html/MediaController.h"
 #include "core/html/TimeRanges.h"
 #include "core/html/shadow/MediaControls.h"
+#include "core/html/track/TextTrackList.h"
 #include "core/layout/LayoutSlider.h"
 #include "core/layout/LayoutTheme.h"
 #include "core/layout/LayoutVideo.h"
 #include "core/page/EventHandler.h"
+#include "platform/EventDispatchForbiddenScope.h"
 #include "platform/RuntimeEnabledFeatures.h"
+#include "platform/text/PlatformLocale.h"
 
 namespace blink {
 
@@ -53,6 +59,12 @@ using namespace HTMLNames;
 // This is the duration from mediaControls.css
 static const double fadeOutDuration = 0.3;
 
+// track index attribute specifies trackIndex for text track list elements
+static const char* trackIndexAttr = "x-webkit-track-index";

[fs, 2015/04/14 12:34:56]

const char foo[] = ...

Also since this should be in a (closed) shadow tree it seems it could be data-
or something instead.

[srivats, 2015/04/16 23:37:19]

I couldn't find the data- convention used for attributes. I used the "x-webkit"
because I saw other parts of code having this. Is there a convention for this?

[fs, 2015/04/17 11:54:46]

I don't think the exact name should matter much - this should be isolated via
the shadow tree anyway. That being said though, I like to think of this as
something that could be implemented in JS, and there you could use data-
attributes (or expandos, or...). Using x-webkit- in that context though would be
weird. So, no strong opinion except for not using x-webkit-.

[srivats, 2015/04/21 01:48:55]

Done.

+
+// when specified as trackIndex, disable text tracks
+static const int trackIndexOffValue = -1;
+
 static bool isUserInteractionEvent(Event* event)
 {
     const AtomicString& type = event->type();
@@ -341,14 +353,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());
-        setChecked(mediaElement().closedCaptionsVisible());
+        mediaControls().toggleClosedCaptionsTrackList();
         updateDisplayType();
         event->setDefaultHandled();
     }
@@ -358,6 +368,145 @@ void MediaControlToggleClosedCaptionsButtonElement::defaultEventHandler(Event* e
 
 // ----------------------------
 
+MediaControlClosedCaptionsTrackListContainerElement::MediaControlClosedCaptionsTrackListContainerElement(MediaControls& mediaControls)
+    : MediaControlDivElement(mediaControls, MediaClosedCaptionsTrackListContainer)
+{
+}
+
+PassRefPtrWillBeRawPtr<MediaControlClosedCaptionsTrackListContainerElement>
+    MediaControlClosedCaptionsTrackListContainerElement::create(MediaControls& mediaControls)
+{
+    RefPtrWillBeRawPtr<MediaControlClosedCaptionsTrackListContainerElement> element =
+        adoptRefWillBeNoop(new MediaControlClosedCaptionsTrackListContainerElement(mediaControls));
+    element->setShadowPseudoId(AtomicString("-webkit-media-controls-closed-captions-track-list-container",
+        AtomicString::ConstructFromLiteral));
+    element->hide();
+    return element.release();
+}
+
+// ----------------------------
+
+MediaControlClosedCaptionsTrackListElement::MediaControlClosedCaptionsTrackListElement(MediaControls& mediaControls)
+    : MediaControlDivElement(mediaControls, MediaClosedCaptionsTrackList)
+{
+}
+
+PassRefPtrWillBeRawPtr<MediaControlClosedCaptionsTrackListElement> MediaControlClosedCaptionsTrackListElement::create(MediaControls& mediaControls)
+{
+    RefPtrWillBeRawPtr<MediaControlClosedCaptionsTrackListElement> element =
+        adoptRefWillBeNoop(new MediaControlClosedCaptionsTrackListElement(mediaControls));
+    element->setShadowPseudoId(AtomicString("-webkit-media-controls-closed-captions-track-list",

[fs, 2015/04/14 12:34:56]

I think we should avoid carrying the "closed captions" term forward where it
doesn't make sense, so:

-webkit-media-controls-text-track-list

might make sense (actually, -internal-media-controls-text-track-list or
soemthing probably makes even more sense.)

[srivats, 2015/04/16 23:37:20]

Changed it here and everywhere in this CL

+        AtomicString::ConstructFromLiteral));
+    element->hide();

[fs, 2015/04/14 12:34:56]

The container is already hidden, so this doesn't seem to add anything useful.

[srivats, 2015/04/16 23:37:20]

Done.

+    return element.release();
+}
+
+void MediaControlClosedCaptionsTrackListElement::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;
+
+        bool validIndex;
+        int trackIndex = toElement(target)->getAttribute(trackIndexAttr).toInt(&validIndex);
+        if (!validIndex)
+            return;
+        bool isVisible = trackIndex != trackIndexOffValue;
+        if (isVisible && trackIndex >= 0)
+            mediaElement().showTextTrackAtIndex(trackIndex);
+
+        mediaElement().setClosedCaptionsVisible(isVisible);

[fs, 2015/04/14 12:34:56]

If we need to call into HTMLMediaElement (directly), then it seems once should
be enough. Also it seems the flag will be true even if the predicate guarding
showTextTrackAtIndex is false (when will trackIndex be < 0 and not ==
trackIndexOffValue?)

[srivats, 2015/04/16 23:37:19]

Moved showTextTrackAtIndex to this file and setting closedCaptionsVisible
appropriately.

+
+        mediaControls().toggleClosedCaptionsTrackList();
+        event->setDefaultHandled();
+    }
+}

[fs, 2015/04/14 12:34:57]

Call the parent class?

[srivats, 2015/04/16 23:37:19]

Done.

+
+String MediaControlClosedCaptionsTrackListElement::getTextTrackLabel(TextTrack* track)
+{
+    String trackLabel;
+
+    if (!(track->label().isNull() || track->label().isEmpty())) {

[fs, 2015/04/14 12:34:56]

isEmpty implies isNull, so this can be simplified to just isEmpty. Also, I think
it could be "denested" a bit:

String trackLabel = track->label();
if (trackLabel.isEmpty())
  trackLabel = track->language();
if (trackLabel.isEmpty())
  trackLabel = ...

[srivats, 2015/04/16 23:37:19]

Done.

+        trackLabel = track->label();
+    } else {
+        if (!(track->language().isNull() || track->language().isEmpty()))
+            trackLabel = track->language();
+        else
+            trackLabel = String(mediaElement().locale().queryString(WebLocalizedString::TextTracksNoLabel));
+    }
+
+    // When the track kind is captions, add a captions marker to distinguish between captions and subtitles.
+    if (track->kind() == track->captionsKeyword())
+        trackLabel.append(String(mediaElement().locale().queryString(WebLocalizedString::TextTracksCaptionsMarker)));

[fs, 2015/04/14 12:34:56]

Locale::queryString returns a String, so this cast should be unnecessary.

[srivats, 2015/04/16 23:37:19]

Done.

+    return trackLabel;
+}
+
+RefPtrWillBeRawPtr<Element> MediaControlClosedCaptionsTrackListElement::createTextTrackListItem(const String& trackDisplayText, int trackIndex, bool isShowing)
+{
+    Document* doc = &document();

[fs, 2015/04/14 12:34:56]

Prefer a reference. Also 'document' rather than 'doc'.

[srivats, 2015/04/16 23:37:19]

Changed the variable name to document and changed to reference. I had to call
this->document() instead of just document() because the compiler complains when
the method name and the variable name are the same.

+    RefPtrWillBeRawPtr<Element> trackItem = doc->createElement(labelTag, false);

[fs, 2015/04/14 12:34:56]

You can call the create() functions directly instead of doing this.

[srivats, 2015/04/16 23:37:20]

Calling Element::create() doesn't create an HTML element and hence results in
ASSERTION FAILED: element.isStyledElement()

[fs, 2015/04/17 11:54:46]

I believe for this call it'd be HTMLLabelElement::create(), and similarly for
other HTML elements.

[srivats, 2015/04/21 01:48:55]

Done.

+    trackItem->setShadowPseudoId(AtomicString("-webkit-media-controls-closed-captions-track-list-item",
+        AtomicString::ConstructFromLiteral));
+    RefPtrWillBeRawPtr<Element> trackItemInput = doc->createElement(inputTag, false);
+    trackItemInput->setShadowPseudoId(AtomicString("-webkit-media-controls-closed-captions-track-list-item-input",
+        AtomicString::ConstructFromLiteral));
+    trackItemInput->setAttribute(typeAttr, InputTypeNames::radio);
+    trackItemInput->setAttribute(nameAttr, "tracks");
+    trackItemInput->setAttribute(trackIndexAttr, AtomicString::number(trackIndex), ASSERT_NO_EXCEPTION);
+    if (!mediaElement().closedCaptionsVisible()) {
+        if (trackIndex == trackIndexOffValue)
+            toHTMLInputElement(trackItemInput)->setChecked(true);
+    } else {
+        if (trackIndex >= 0 && isShowing)

[fs, 2015/04/14 12:34:57]

Looks like this means multiple tracks can be selected - yet the method enabling
tracks disable all other tracks?

[srivats, 2015/04/16 23:37:19]

Since the track list is a list of input radio, it's possible to select only one
track.

[fs, 2015/04/17 11:54:46]

This is not guaranteed by this expression though AFAICS (the track mode can be
affected outside the UI after all.) So I think you should at least test with
multiple showing tracks and see what happens.

[srivats, 2015/04/21 01:48:55]

When multiple tracks are set to showing, it displays multiple text tracks on the
display but shows only the highest track index checked on the menu as I
suspected would happen. I'll think a bit on how to handle this case properly.

+            toHTMLInputElement(trackItemInput)->setChecked(true);
+    }
+    trackItem->appendChild(trackItemInput);
+    trackItem->appendChild(doc->createTextNode(trackDisplayText));
+    return trackItem;
+}
+
+void MediaControlClosedCaptionsTrackListElement::refreshTextTrackListMenu()
+{
+    if (!mediaElement().hasClosedCaptions() || !mediaElement().textTracksAreReady())
+        return;
+
+    TextTrackList* trackList = mediaElement().textTracks();
+    if (!trackList || trackList->length() == 0)

[fs, 2015/04/14 12:34:56]

If hasClosedCaptions() returns true, then there'll be a track list with at least
one relevant track I believe.

[srivats, 2015/04/16 23:37:19]

Good point. Removed.

+        return;
+
+    removeChildren();
+
+    // Construct a menu for subtitles and captions
+    EventDispatchForbiddenScope::AllowUserAgentEvents allowEvents;
+    Document* doc = &document();
+    RefPtrWillBeRawPtr<Element> tracksHeader = doc->createElement(h3Tag, false);
+    tracksHeader->setShadowPseudoId(AtomicString("-webkit-media-controls-closed-captions-track-list-header",
+        AtomicString::ConstructFromLiteral));
+    RefPtrWillBeRawPtr<Element> tracksSection = doc->createElement(divTag, false);
+
+    tracksHeader->appendChild(doc->createTextNode(mediaElement().locale().queryString(WebLocalizedString::TextTracksSubtitlesCC)));

[fs, 2015/04/14 12:34:56]

What's the 'CC' suffix for - seems redundant unless it should be
SubtitlesAndCaptions or something like that.

[srivats, 2015/04/16 23:37:19]

I wasn't sure what the header should say. Safari uses just Subtitles and YouTube
uses Subtitles/CC so went with this.

[fs, 2015/04/17 11:54:46]

Ok, so it's SubtitlesOrCaptions then. What the string actually says would be up
to localization I think (for instance the "CC" term may not make sense in all
parts of the world et.c). I.e. it could make sense to chose the slightly wider
term on this side of things.

[srivats, 2015/04/21 01:48:55]

That makes sense. I feel Subtitles And/Or Captions will be too long a string on
the header. Let me know if you are okay with making it just "Subtitles"

[fs, 2015/04/21 12:13:34]

Sounds OK to me.

[srivats, 2015/04/21 21:11:18]

Done.

+    AtomicString tracksSectionId = AtomicString("tracks-header");

[fs, 2015/04/14 12:34:56]

ConstructFromLiteral

[srivats, 2015/04/16 23:37:19]

Done.

+    tracksHeader->setAttribute(idAttr, tracksSectionId);
+    tracksSection->setAttribute(roleAttr, radiogroupAttr.localName());
+    tracksSection->setAttribute(aria_labeledbyAttr, tracksSectionId);
+
+    String textTracksOffLabel = String(mediaElement().locale().queryString(WebLocalizedString::TextTracksOff));

[fs, 2015/04/14 12:34:56]

queryString returns a String.

[srivats, 2015/04/16 23:37:19]

Done.

+    tracksSection->appendChild(createTextTrackListItem(textTracksOffLabel, trackIndexOffValue, false));
+
+    for (unsigned i = 0; i < trackList->length(); i++) {
+        TextTrack* track = trackList->item(i);
+        RefPtrWillBeRawPtr<Element> trackItem =
+            createTextTrackListItem(getTextTrackLabel(track), i, track->mode() == TextTrack::showingKeyword());

[fs, 2015/04/14 12:34:56]

TextTrack::trackIndex() can already give 'i', so it almost looks like it would
be easier to just pass the entire track to createTextTrackListItem().

Should also filter out non-rendered tracks.

[srivats, 2015/04/16 23:37:19]

Passing in TextTrack won't help with reusing this method for creating the "Off"
item. By non-rendered tracks, are you referring to the tracks that don't have a
ready state of LOADED?

[fs, 2015/04/17 11:54:46]

I guess that's a fair point to make, although I guess that could easily be
represented using 'nullptr'.
See the similar comment elsewhere.

[srivats, 2015/04/21 01:48:55]

Done.

+        tracksSection->appendChild(trackItem);
+    }
+    appendChild(tracksHeader);
+    appendChild(tracksSection);
+}
+
+// ----------------------------
+
 MediaControlTimelineElement::MediaControlTimelineElement(MediaControls& mediaControls)
     : MediaControlInputElement(mediaControls, MediaSlider)
 {