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 c66ef9e0741eaa2fc9ac05c002b2042be9c8fdac..90e9aa90e11a11ab687e872cf96097a2507cec2d 100644
--- a/Source/core/html/shadow/MediaControlElements.cpp
+++ b/Source/core/html/shadow/MediaControlElements.cpp
@@ -33,19 +33,27 @@
 #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/Fullscreen.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/HTMLHeadingElement.h"
+#include "core/html/HTMLLabelElement.h"
 #include "core/html/HTMLVideoElement.h"
 #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 {
 
@@ -54,6 +62,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

[philipj_slow, 2015/05/05 14:36:57]

This comment doesn't explain what this is for. AFAICT, it's to save the text
track index to avoid holding a pointer to the TextTrack directly.

https://www.chromium.org/blink/coding-style#TOC-Comments isn't very clear on
this, but most comments are capitalized and punctuated.

[srivats, 2016/02/23 01:39:26]

Done.

+static const char trackIndexAttr[] = "data-webkit-track-index";

[philipj_slow, 2015/05/05 14:36:57]

Since the element is inaccessible to the outside, any name at all would do. It
doesn't need webkit in the name, so just "data-track-index" is fine.

[srivats, 2016/02/23 01:39:27]

Done.

+
+// when specified as trackIndex, disable text tracks

[philipj_slow, 2015/05/05 14:36:57]

Is a special value really needed for this, couldn't the absence of the attribute
be used instead?

[srivats, 2016/02/23 01:39:26]

The -1 id helps with testing mostly to track the "Off" item in the menu.

+static const int trackIndexOffValue = -1;
+
 static bool isUserInteractionEvent(Event* event)
 {
     const AtomicString& type = event->type();
@@ -342,14 +356,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().toggleTextTrackList();
         updateDisplayType();
         event->setDefaultHandled();
     }
@@ -359,6 +371,142 @@ 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->hide();
+    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;
+
+        bool validIndex;
+        int trackIndex = toElement(target)->getAttribute(trackIndexAttr).toInt(&validIndex);
+        ASSERT(validIndex);
+        if (trackIndex != trackIndexOffValue) {
+            showTextTrackAtIndex(trackIndex);

[philipj_slow, 2015/05/05 14:36:56]

What happens if the text tracks change while this menu is showing? It looks like
the wrong track will be enabled?

[srivats, 2016/02/23 01:39:27]

I tested this out by removing a track using the console while the menu was open
and clicking on the track just removed, the next track on the list got enabled.
This does seem inconvenient, but the other option would be to refresh the menu
when this happens. Would you recommend that?

[philipj_slow, 2016/03/01 11:21:04]

Refreshing the menu also seems wrong, maybe the user isn't even interested in
that specific track and yet the UI jumps around. I think the best one could hope
for is a behavior equivalent to what happens with a text track with a broken
URL, which is that one can click it, the other tracks are disabled, but then
nothing happens. However, that seems like a bit of overkill, I suggest waiting
until it's a real problem.

+            mediaElement().setClosedCaptionsVisible(true);

[philipj_slow, 2015/05/05 14:36:57]

I think setClosedCaptionsVisible() needs to change a bit. It made sense for the
single on/off CC button, but now I don't think there should be any way for the
media controls to invoke automatic text track selection at all. In fact, I'm not
sure why HTMLMediaElement needs to be notified here at all. What is the
difference between this code and a track picker implemented in JavaScript? Both
should behave the same, IMHO. In other words, it's TextTrack::setMode that
should be the shared entry-point.

[fs, 2015/05/05 15:32:44]

I agree with this, but I think we need a "step" in between.
https://codereview.chromium.org/1118613002/ seems like it could be that step.

[philipj_slow, 2015/05/06 09:28:43]

Ahem, will get to that review too...

[srivats, 2016/02/23 01:39:27]

Not using setClosedCaptionsVisible anymore and it's currently not being used
anywhere. If you don't forsee any other use case for it, I can get rid of the
method.

+        } else {
+            mediaElement().setClosedCaptionsVisible(false);
+        }
+
+        mediaControls().toggleTextTrackList();
+        event->setDefaultHandled();
+    }
+    HTMLDivElement::defaultEventHandler(event);
+}
+
+void MediaControlTextTrackListElement::showTextTrackAtIndex(unsigned indexToEnable)
+{
+    TextTrackList* trackList = mediaElement().textTracks();
+    for (unsigned i = 0; i < trackList->length(); ++i) {
+        TextTrack* track = trackList->item(i);
+        if (!track->isRenderable())

[philipj_slow, 2015/05/05 14:36:57]

Is this really needed?

[fs, 2015/05/05 15:32:44]

I probably suggested this in one of the earlier CLs... If this method will only
ever accept indices extracted from the list (and assuming no mutations...) this
wouldn't be needed. I think that's the case now (mutations excluded.)

+            continue;
+        if (i == indexToEnable && indexToEnable < trackList->length()) {

[philipj_slow, 2015/05/05 14:36:57]

The indexToEnable < trackList->length() check could be moved out of the loop.

[srivats, 2016/02/23 01:39:27]

Done.

+            track->setMode(TextTrack::showingKeyword());
+            mediaElement().disableAutomaticTextTrackSelection();

[philipj_slow, 2015/05/05 14:36:57]

This is the bit I'm saying should happen the same for this and a page-provided
UI as some side-effect of TextTrack::setMode.

+        } else 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, 2015/05/05 14:36:56]

This seems risky. People could use <track srclang="English"> and have English
show up in this menu, making them think this is the intended usage. If anything,
I think the BCP 47 label needs to be mapped to the name of the language in the
language itself (N strings) or the UA language (N*M strings).

[fs, 2015/05/05 15:32:44]

ICU quite possibly have that data already. No idea how to dig it out though =)

+
+    if (trackLabel.isEmpty())
+        trackLabel = String(mediaElement().locale().queryString(WebLocalizedString::TextTracksNoLabel));
+
+    // When the track kind is captions, add a captions marker to distinguish between captions and subtitles.

[philipj_slow, 2015/05/05 14:36:57]

I'm not sure this will make sense in all languages, as a Swedish user I wouldn't
know what [CC] means, at least. I'd probably realize that it has to do with
subtitles because the button says so, but that it's the difference between
subtitles and captions (for hard-of-hearing) is not obvious. I don't have a good
suggestion here, but think we need to involve some UI people in this.

[fs, 2015/05/05 15:32:44]

Since it is localized, it could well be "" for Swedish, "(HoH)" for en-GB etc.

[srivats, 2016/02/23 01:39:27]

UX suggested I use icons here to differentiate between the two track kinds

[philipj_slow, 2016/03/01 11:21:04]

And I really like that new subtitles icon!

+    if (track->kind() == track->captionsKeyword())
+        trackLabel.append(mediaElement().locale().queryString(WebLocalizedString::TextTracksCaptionsMarker));
+    return trackLabel;
+}
+
+RefPtrWillBeRawPtr<Element> MediaControlTextTrackListElement::createTextTrackListItem(TextTrack* track)

[philipj_slow, 2015/05/05 14:36:57]

This is a bit hard to understand, what does it mean to pass nullptr for the
track argument? Can you document that this is to create the "Off" line in the
list of tracks?

[fs, 2015/05/05 15:32:44]

I believe I suggested to pass a track, because the parametrized version looked
messy to me =)

Maybe a small helper builder would be a good middle-ground solution.

[philipj_slow, 2015/05/06 09:28:43]

Sorry, I skipped the history and comments, so we're touching on the same areas
with opposing suggestions :)

[srivats, 2016/02/23 01:39:26]

Added a comment explaining what the nullptr is for.

+{
+    Document& document = this->document();
+    int trackIndex = track ? track->trackIndex() : trackIndexOffValue;
+    RefPtrWillBeRawPtr<HTMLLabelElement> trackItem = HTMLLabelElement::create(document, 0);
+    trackItem->setShadowPseudoId(AtomicString("-internal-media-controls-text-track-list-item",
+        AtomicString::ConstructFromLiteral));
+    RefPtrWillBeRawPtr<HTMLInputElement> trackItemInput = HTMLInputElement::create(document, 0, false);
+    trackItemInput->setShadowPseudoId(AtomicString("-internal-media-controls-text-track-list-item-input",
+        AtomicString::ConstructFromLiteral));
+    trackItemInput->setType(InputTypeNames::radio);
+    trackItemInput->setAttribute(trackIndexAttr, AtomicString::number(trackIndex), ASSERT_NO_EXCEPTION);
+    if (!mediaElement().closedCaptionsVisible()) {
+        if (!track)
+            trackItemInput->setChecked(true);
+    } else {
+        if (track && track->mode() == TextTrack::showingKeyword())

[philipj_slow, 2015/05/05 14:36:57]

If there are multiple tracks showing, I guess they will all have a checkmark?
Perhaps document this to make it explicit.

[srivats, 2016/02/23 01:39:27]

Yes. I tested this case out.

+            trackItemInput->setChecked(true);
+    }
+    trackItem->appendChild(trackItemInput);
+    trackItem->appendChild(Text::create(document, getTextTrackLabel(track)));
+    return trackItem;
+}
+
+void MediaControlTextTrackListElement::refreshTextTrackListMenu()
+{
+    DEFINE_STATIC_LOCAL(const AtomicString, tracksSectionId, ("tracks-header", AtomicString::ConstructFromLiteral));
+    if (!mediaElement().hasClosedCaptions() || !mediaElement().textTracksAreReady())
+        return;
+
+    removeChildren();
+
+    // Construct a menu for subtitles and captions
+    EventDispatchForbiddenScope::AllowUserAgentEvents allowEvents;
+    Document& document = this->document();
+    RefPtrWillBeRawPtr<HTMLHeadingElement> tracksHeader = HTMLHeadingElement::create(h3Tag, document);
+    tracksHeader->setShadowPseudoId(AtomicString("-internal-media-controls-text-track-list-header",
+        AtomicString::ConstructFromLiteral));
+    RefPtrWillBeRawPtr<HTMLDivElement> tracksSection = HTMLDivElement::create(document);
+
+    tracksHeader->appendChild(Text::create(document,
+        mediaElement().locale().queryString(WebLocalizedString::TextTracksSubtitles)));

[philipj_slow, 2015/05/05 14:36:57]

Is this line needed at all? Even if it were changed to "Captions" for
consistency with the "CC" icon, it still seems like a bit much to me. The icon
itself should make it clear that this has to do with text tracks. Again, this is
something we need feedback from UI people on.

[srivats, 2016/02/23 01:39:27]

I got some UX feedback on this and they're fine with getting rid of the header.

+    tracksHeader->setAttribute(idAttr, tracksSectionId);
+    tracksSection->setAttribute(roleAttr, radiogroupAttr.localName());
+    tracksSection->setAttribute(aria_labeledbyAttr, tracksSectionId);

[philipj_slow, 2015/05/05 14:36:56]

Won't we end up with multiple elements with the same id if there are many video
elements? They'll all be in different shadow trees, but is aria-labeledby scoped
to the shadow tree it is in? (This question will evaporate if there is no header
of course.)

[fs, 2015/05/05 15:32:44]

I think it would be weird if it wasn't scoped...

[philipj_slow, 2015/05/06 09:28:43]

Yeah, it certainly would be, just want to make sure that aria-labeledby actually
works in shadow trees today. It's easy to imagine an incorrect implementation
along the lines of element.document().getElementById(...). A test that would
fail if this line is removed would be ideal, if not too hard to write.

[srivats, 2016/02/23 01:39:27]

No more header.

+
+    tracksSection->appendChild(createTextTrackListItem(nullptr));

[philipj_slow, 2015/05/05 14:36:57]

So this is where it looks a bit weird. Maybe if the label, selected state and
track index (if any) is passed into this creator method it would be more clear
what's going on.

[srivats, 2016/02/23 01:39:26]

I added a comment explaining what the nullptr is for. If you still prefer
passing in label, state and index separately, I can change it back to that.

+
+    TextTrackList* trackList = mediaElement().textTracks();
+    for (unsigned i = 0; i < trackList->length(); i++) {
+        TextTrack* track = trackList->item(i);
+        if (!track->isRenderable())
+            continue;
+        RefPtrWillBeRawPtr<Element> trackItem = createTextTrackListItem(track);
+        tracksSection->appendChild(trackItem);
+    }
+    appendChild(tracksHeader);
+    appendChild(tracksSection);
+}
+
+// ----------------------------
+
 MediaControlTimelineElement::MediaControlTimelineElement(MediaControls& mediaControls)
     : MediaControlInputElement(mediaControls, MediaSlider)
 {