Support text track selection in video controls

Support text track selection in video controls

Enable the user to select between text tracks with a menu
that is displayed when the CC button is clicked.

Chromium-side CL: https://codereview.chromium.org/1079323002/

BUG=353105
TEST=run-webkit-tests media*

[srivats, 2015-04-11 00:57:26]

srivats@amazon.com changed reviewers:
+ philipj@opera.com

[srivats, 2015-04-11 00:59:34]

Hi Philip,

I've added the text track selection menu to Blink. PTAL.

FYI I encountered some layout test failures when I set overflow to visible on
video::-webkit-media-controls-panel which is required to display the track
selection menu. I'm currently setting overflow to visible only when the track
selection menu is displayed in MediaControls.cpp to avoid updating the layout
test images. Please let me know if there is a better way to deal with this.

Thanks,
Sunitha

[fs, 2015-04-14 12:34:54]

fs@opera.com changed reviewers:
+ fs@opera.com

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

Some comments while you're waiting for philipj.

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
File Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:334: -webkit-appearance:
media-closed-captions-track-list-container;
I didn't see an appearance implemented for this - remove?

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:343: z-index: 1;
Is this really needed?

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:345: overflow-y: auto;
Have you tested that this works out as expected (and looks ok - I imagine a
normal scrollbar would look a bit odd in this context...)

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:356: max-height: 250px;
I take it this is supposed to be "10 items", but how does this interact with the
container (and do we really need both of them?)

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:357: text-align: center;
Is this supposed to affect the header items?

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:376: font-size: 16px;
Looks like font-size could be set higher up and inherited here instead?

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:377: text-align: left;
Why not just use the default (which will be 'left' if LTR.)

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:388: position: absolute;
Do we really need to abspos this?

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:389: visibility: none;
s/none/hidden/

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:396: visibility: visible;
Why not have just the visibility toggle? Do we want the dimensions to change
when hovering?

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/HTMLMediaE...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.cpp:1171: void
HTMLMediaElement::showTextTrackAtIndex(unsigned index)
Any reason - m_selectedTextTrack excepted - to have this in HTMLMediaElement?

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.cpp:1179:
track->setMode(TextTrack::disabledKeyword());
This ought to be more selective. I.e. only do this if the track is of a rendered
kind and "showing" - even the latter is not entirely obvious either.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.cpp:1183: m_selectedTextTrack =
trackList->item(index);
It feels a bit like this is only used to avoid running auto-selection - which a
single bit would suffice for (and IIRC that's what the spec has too.) The bits
involved in text track selection already are a bit of a mess though.
Quite possibly the bit could be set based on a gesture rather than a direct call
too.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControlElementTypes.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElementTypes.cpp:95: return (!propertySet ||
!propertySet->getPropertyCSSValue(CSSPropertyDisplay));
Redundant (). Use hasProperty()?

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:63: static const char*
trackIndexAttr = "x-webkit-track-index";
const char foo[] = ...

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

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:398:
element->setShadowPseudoId(AtomicString("-webkit-media-controls-closed-captions-track-list",
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.)

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:400: element->hide();
The container is already hidden, so this doesn't seem to add anything useful.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:420:
mediaElement().setClosedCaptionsVisible(isVisible);
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?)

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:425: }
Call the parent class?

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:431: if
(!(track->label().isNull() || track->label().isEmpty())) {
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 = ...

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:442:
trackLabel.append(String(mediaElement().locale().queryString(WebLocalizedString::TextTracksCaptionsMarker)));
Locale::queryString returns a String, so this cast should be unnecessary.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:448: Document* doc =
&document();
Prefer a reference. Also 'document' rather than 'doc'.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:449:
RefPtrWillBeRawPtr<Element> trackItem = doc->createElement(labelTag, false);
You can call the create() functions directly instead of doing this.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:462: if (trackIndex >= 0 &&
isShowing)
Looks like this means multiple tracks can be selected - yet the method enabling
tracks disable all other tracks?

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:476: if (!trackList ||
trackList->length() == 0)
If hasClosedCaptions() returns true, then there'll be a track list with at least
one relevant track I believe.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:489:
tracksHeader->appendChild(doc->createTextNode(mediaElement().locale().queryString(WebLocalizedString::TextTracksSubtitlesCC)));
What's the 'CC' suffix for - seems redundant unless it should be
SubtitlesAndCaptions or something like that.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:490: AtomicString
tracksSectionId = AtomicString("tracks-header");
ConstructFromLiteral

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:495: String textTracksOffLabel
=
String(mediaElement().locale().queryString(WebLocalizedString::TextTracksOff));
queryString returns a String.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:501:
createTextTrackListItem(getTextTrackLabel(track), i, track->mode() ==
TextTrack::showingKeyword());
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.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.h:34: #include
"core/html/track/TextTrack.h"
Could be a forward declaration.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.h:150: class
MediaControlClosedCaptionsTrackListContainerElement final : public
MediaControlDivElement {
This type does not add anything AFAICS, so why do we need it?

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControls.cpp:372: if
(!mediaElement().hasClosedCaptions() || !m_closedCaptionsTrackListContainer)
m_closedCaptionsTrackListContainer is always created/set when creating the
controls, so this check seems redundant.

Also, what if the tracks are removed when the menu is open - what handles that.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControls.cpp:376: // Media controls panel has
overflow set to hidden. To display track list,
Maybe this is an indication that the menu should not be within the panel then?

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControls.h:62: void
toggleClosedCaptionsTrackList();
Prefer a name without "ClosedCaptions" (toggleTextTrackSelectionList or
something maybe)

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

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
File Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:334: -webkit-appearance:
media-closed-captions-track-list-container;
On 2015/04/14 12:34:55, fs wrote:
> I didn't see an appearance implemented for this - remove?

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:343: z-index: 1;
On 2015/04/14 12:34:55, fs wrote:
> Is this really needed?

This wasn't required. I removed it.

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:345: overflow-y: auto;
On 2015/04/14 12:34:56, fs wrote:
> Have you tested that this works out as expected (and looks ok - I imagine a
> normal scrollbar would look a bit odd in this context...)

I had tested it earlier but didn't really know how to customize the scrollbar.
Figured it out now

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:356: max-height: 250px;
On 2015/04/14 12:34:55, fs wrote:
> I take it this is supposed to be "10 items", but how does this interact with
the
> container (and do we really need both of them?)
Removed this one.

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:357: text-align: center;
On 2015/04/14 12:34:55, fs wrote:
> Is this supposed to affect the header items?

Yes it's only for the header. I moved it below.

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:376: font-size: 16px;
On 2015/04/14 12:34:55, fs wrote:
> Looks like font-size could be set higher up and inherited here instead?

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:377: text-align: left;
On 2015/04/14 12:34:56, fs wrote:
> Why not just use the default (which will be 'left' if LTR.)

Leaving this unspecified actually right aligned the text because the panel has
text-align set to right.
Even if the text-align:right on panel goes away, we should keep the
text-align:left on the track-list item. This is because we might come across
webpages that have text-align set to center for a video and the track-list-item
will end up inheriting that.

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:388: position: absolute;
On 2015/04/14 12:34:55, fs wrote:
> Do we really need to abspos this?

I positioned this absolutely so that the track labels aligned correctly. An
alternate approach could be to give all elements a fixed margin-left and style
the selected item to not use a margin-left. Thoughts?

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:389: visibility: none;
On 2015/04/14 12:34:55, fs wrote:
> s/none/hidden/

Changed visibility to hidden and moved the dimensions to here.

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:396: visibility: visible;
On 2015/04/14 12:34:55, fs wrote:
> Why not have just the visibility toggle? Do we want the dimensions to change
> when hovering?

Moved it up.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/HTMLMediaE...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.cpp:1171: void
HTMLMediaElement::showTextTrackAtIndex(unsigned index)
On 2015/04/14 12:34:56, fs wrote:
> Any reason - m_selectedTextTrack excepted - to have this in HTMLMediaElement?

No. I moved this method to MediaControlElements and now I'm setting a bit to
prevent automatic track selection from happening when a track is selected by the
user.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.cpp:1179:
track->setMode(TextTrack::disabledKeyword());
On 2015/04/14 12:34:56, fs wrote:
> This ought to be more selective. I.e. only do this if the track is of a
rendered
> kind and "showing" - even the latter is not entirely obvious either.

By rendered tracks, are you referring to tracks with ready state LOADED? I added
a check to disable a track only if it's showing.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.cpp:1183: m_selectedTextTrack =
trackList->item(index);
On 2015/04/14 12:34:56, fs wrote:
> It feels a bit like this is only used to avoid running auto-selection - which
a
> single bit would suffice for (and IIRC that's what the spec has too.) The bits
> involved in text track selection already are a bit of a mess though.
> Quite possibly the bit could be set based on a gesture rather than a direct
call
> too.

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControlElementTypes.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElementTypes.cpp:95: return (!propertySet ||
!propertySet->getPropertyCSSValue(CSSPropertyDisplay));
On 2015/04/14 12:34:56, fs wrote:
> Redundant (). Use hasProperty()?

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:63: static const char*
trackIndexAttr = "x-webkit-track-index";
On 2015/04/14 12:34:56, fs wrote:
> const char foo[] = ...
> 
> Also since this should be in a (closed) shadow tree it seems it could be data-
> or something instead.

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?

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:398:
element->setShadowPseudoId(AtomicString("-webkit-media-controls-closed-captions-track-list",
On 2015/04/14 12:34:56, fs wrote:
> 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.)

Changed it here and everywhere in this CL

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:400: element->hide();
On 2015/04/14 12:34:56, fs wrote:
> The container is already hidden, so this doesn't seem to add anything useful.

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:420:
mediaElement().setClosedCaptionsVisible(isVisible);
On 2015/04/14 12:34:56, fs wrote:
> 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?)

Moved showTextTrackAtIndex to this file and setting closedCaptionsVisible
appropriately.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:425: }
On 2015/04/14 12:34:57, fs wrote:
> Call the parent class?

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:431: if
(!(track->label().isNull() || track->label().isEmpty())) {
On 2015/04/14 12:34:56, fs wrote:
> 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 = ...

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:442:
trackLabel.append(String(mediaElement().locale().queryString(WebLocalizedString::TextTracksCaptionsMarker)));
On 2015/04/14 12:34:56, fs wrote:
> Locale::queryString returns a String, so this cast should be unnecessary.

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:448: Document* doc =
&document();
On 2015/04/14 12:34:56, fs wrote:
> Prefer a reference. Also 'document' rather than 'doc'.
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.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:449:
RefPtrWillBeRawPtr<Element> trackItem = doc->createElement(labelTag, false);
On 2015/04/14 12:34:56, fs wrote:
> You can call the create() functions directly instead of doing this.

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

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:462: if (trackIndex >= 0 &&
isShowing)
On 2015/04/14 12:34:57, fs wrote:
> Looks like this means multiple tracks can be selected - yet the method
enabling
> tracks disable all other tracks?

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

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:476: if (!trackList ||
trackList->length() == 0)
On 2015/04/14 12:34:56, fs wrote:
> If hasClosedCaptions() returns true, then there'll be a track list with at
least
> one relevant track I believe.

Good point. Removed.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:489:
tracksHeader->appendChild(doc->createTextNode(mediaElement().locale().queryString(WebLocalizedString::TextTracksSubtitlesCC)));
On 2015/04/14 12:34:56, fs wrote:
> What's the 'CC' suffix for - seems redundant unless it should be
> SubtitlesAndCaptions or something like that.

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

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:490: AtomicString
tracksSectionId = AtomicString("tracks-header");
On 2015/04/14 12:34:56, fs wrote:
> ConstructFromLiteral

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:495: String textTracksOffLabel
=
String(mediaElement().locale().queryString(WebLocalizedString::TextTracksOff));
On 2015/04/14 12:34:56, fs wrote:
> queryString returns a String.

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:501:
createTextTrackListItem(getTextTrackLabel(track), i, track->mode() ==
TextTrack::showingKeyword());
On 2015/04/14 12:34:56, fs wrote:
> 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.
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?

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.h:34: #include
"core/html/track/TextTrack.h"
On 2015/04/14 12:34:57, fs wrote:
> Could be a forward declaration.

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.h:150: class
MediaControlClosedCaptionsTrackListContainerElement final : public
MediaControlDivElement {
On 2015/04/14 12:34:57, fs wrote:
> This type does not add anything AFAICS, so why do we need it?

It's the container that holds the track list. I'll see if it's possible to do
away with it.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControls.cpp:372: if
(!mediaElement().hasClosedCaptions() || !m_closedCaptionsTrackListContainer)
On 2015/04/14 12:34:57, fs wrote:
> m_closedCaptionsTrackListContainer is always created/set when creating the
> controls, so this check seems redundant.
> 
Removed

> Also, what if the tracks are removed when the menu is open - what handles
that.

If the tracks are removed while the menu is open, the user can still click on
the items in the track list but they don't do anything functionally(shows
checkmark). Once the menu is dismissed, you can't bring it back up again unless
new tracks are added cause there is no CC button anymore and no tracks to show.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControls.cpp:376: // Media controls panel has
overflow set to hidden. To display track list,
On 2015/04/14 12:34:57, fs wrote:
> Maybe this is an indication that the menu should not be within the panel then?

Seems logical to be a part of media controls because the CC button brings this
menu up. Do you have an alternate approach for this?

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControls.h:62: void
toggleClosedCaptionsTrackList();
On 2015/04/14 12:34:57, fs wrote:
> Prefer a name without "ClosedCaptions" (toggleTextTrackSelectionList or
> something maybe)

Done.

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

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
File Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:377: text-align: left;
On 2015/04/16 23:37:19, srivats wrote:
> On 2015/04/14 12:34:56, fs wrote:
> > Why not just use the default (which will be 'left' if LTR.)
> 
> Leaving this unspecified actually right aligned the text because the panel has
> text-align set to right.
> Even if the text-align:right on panel goes away, we should keep the
> text-align:left on the track-list item. This is because we might come across
> webpages that have text-align set to center for a video and the
track-list-item
> will end up inheriting that.

Ok, suggest to make this "text-align: start" then which would be left or right
depending on direction.

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:388: position: absolute;
On 2015/04/16 23:37:18, srivats wrote:
> On 2015/04/14 12:34:55, fs wrote:
> > Do we really need to abspos this?
> 
> I positioned this absolutely so that the track labels aligned correctly. An
> alternate approach could be to give all elements a fixed margin-left and style
> the selected item to not use a margin-left. Thoughts?

I'd prefer a non-abspos version.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/HTMLMediaE...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.cpp:1179:
track->setMode(TextTrack::disabledKeyword());
On 2015/04/16 23:37:19, srivats wrote:
> On 2015/04/14 12:34:56, fs wrote:
> > This ought to be more selective. I.e. only do this if the track is of a
> rendered
> > kind and "showing" - even the latter is not entirely obvious either.
> 
> By rendered tracks, are you referring to tracks with ready state LOADED? I
added
> a check to disable a track only if it's showing.

No, I mean tracks that are either kind=subtitles or kind=captions. See for
instance TextTrack::isRendered(). Adding a new method "isVisual" or
"isRenderable" or something like that might make sense, and then you could use
that here.

Checking LOADED would not be appropriate, but it does add additional
complication since loading a track could fail, so tracks with readinessState()
== FailedToLoad should probably be excluded as well.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:63: static const char*
trackIndexAttr = "x-webkit-track-index";
On 2015/04/16 23:37:19, srivats wrote:
> On 2015/04/14 12:34:56, fs wrote:
> > const char foo[] = ...
> > 
> > Also since this should be in a (closed) shadow tree it seems it could be
data-
> > or something instead.
> 
> 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?

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

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:449:
RefPtrWillBeRawPtr<Element> trackItem = doc->createElement(labelTag, false);
On 2015/04/16 23:37:20, srivats wrote:
> On 2015/04/14 12:34:56, fs wrote:
> > You can call the create() functions directly instead of doing this.
> 
> Calling Element::create() doesn't create an HTML element and hence results in
> ASSERTION FAILED: element.isStyledElement()

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

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:462: if (trackIndex >= 0 &&
isShowing)
On 2015/04/16 23:37:19, srivats wrote:
> On 2015/04/14 12:34:57, fs wrote:
> > Looks like this means multiple tracks can be selected - yet the method
> enabling
> > tracks disable all other tracks?
> 
> Since the track list is a list of input radio, it's possible to select only
one
> track.

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.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:489:
tracksHeader->appendChild(doc->createTextNode(mediaElement().locale().queryString(WebLocalizedString::TextTracksSubtitlesCC)));
On 2015/04/16 23:37:19, srivats wrote:
> On 2015/04/14 12:34:56, fs wrote:
> > What's the 'CC' suffix for - seems redundant unless it should be
> > SubtitlesAndCaptions or something like that.
> 
> I wasn't sure what the header should say. Safari uses just Subtitles and
YouTube
> uses Subtitles/CC so went with this.

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.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:501:
createTextTrackListItem(getTextTrackLabel(track), i, track->mode() ==
TextTrack::showingKeyword());
On 2015/04/16 23:37:19, srivats wrote:
> On 2015/04/14 12:34:56, fs wrote:
> > 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.
> Passing in TextTrack won't help with reusing this method for creating the
"Off"
> item.

I guess that's a fair point to make, although I guess that could easily be
represented using 'nullptr'.

> By non-rendered tracks, are you referring to the tracks that don't have a
> ready state of LOADED?

See the similar comment elsewhere.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControls.cpp:376: // Media controls panel has
overflow set to hidden. To display track list,
On 2015/04/16 23:37:20, srivats wrote:
> On 2015/04/14 12:34:57, fs wrote:
> > Maybe this is an indication that the menu should not be within the panel
then?
> 
> Seems logical to be a part of media controls because the CC button brings this
> menu up. Do you have an alternate approach for this?

Well, first, a menu that's part of the page has a few issue in that it may end
up being clipped by the viewport... (compare for instance with <select>)

Ignoring that though, it should be possible to stack the text track menu on top
of the panel, since its layout is independent (AFAICS). Therë́'s already lots of
other "overlays" present in the control shadow tree.

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:213: void
setAutomaticTextTrackSelection(bool);
This should probably have an "Enabled" suffix to read ok with the bool type.

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:567: bool
m_performAutomaticTextTrackSelection : 1;
Move this up to m_closedCaptionsVisible or the other group of related flags.

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElementTypes.cpp (right):

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElementTypes.cpp:95: return
!propertySet->hasProperty(CSSPropertyDisplay);
Still needs the null-check I believe.

[fs, 2015-04-17 12:06:25]

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:213: void
setAutomaticTextTrackSelection(bool);
On 2015/04/17 11:54:46, fs wrote:
> This should probably have an "Enabled" suffix to read ok with the bool type.

With the below, I think this could just be:

void disableAutomaticTextTrackSelection()

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:567: bool
m_performAutomaticTextTrackSelection : 1;
On 2015/04/17 11:54:46, fs wrote:
> Move this up to m_closedCaptionsVisible or the other group of related flags.

I think we should take the opportunity, and introduce the similar flag from the
spec:

https://html.spec.whatwg.org/#did-perform-automatic-track-selection

So, append 'did' to the name and reverse the value.

[srivats, 2015-04-21 01:48:56]

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
File Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:377: text-align: left;
On 2015/04/17 11:54:46, fs wrote:
> On 2015/04/16 23:37:19, srivats wrote:
> > On 2015/04/14 12:34:56, fs wrote:
> > > Why not just use the default (which will be 'left' if LTR.)
> > 
> > Leaving this unspecified actually right aligned the text because the panel
has
> > text-align set to right.
> > Even if the text-align:right on panel goes away, we should keep the
> > text-align:left on the track-list item. This is because we might come across
> > webpages that have text-align set to center for a video and the
> track-list-item
> > will end up inheriting that.
> 
> Ok, suggest to make this "text-align: start" then which would be left or right
> depending on direction.

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/css/mediaContro...
Source/core/css/mediaControls.css:388: position: absolute;
On 2015/04/17 11:54:46, fs wrote:
> On 2015/04/16 23:37:18, srivats wrote:
> > On 2015/04/14 12:34:55, fs wrote:
> > > Do we really need to abspos this?
> > 
> > I positioned this absolutely so that the track labels aligned correctly. An
> > alternate approach could be to give all elements a fixed margin-left and
style
> > the selected item to not use a margin-left. Thoughts?
> 
> I'd prefer a non-abspos version.

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/HTMLMediaE...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.cpp:1179:
track->setMode(TextTrack::disabledKeyword());
On 2015/04/17 11:54:46, fs wrote:
> On 2015/04/16 23:37:19, srivats wrote:
> > On 2015/04/14 12:34:56, fs wrote:
> > > This ought to be more selective. I.e. only do this if the track is of a
> > rendered
> > > kind and "showing" - even the latter is not entirely obvious either.
> > 
> > By rendered tracks, are you referring to tracks with ready state LOADED? I
> added
> > a check to disable a track only if it's showing.
> 
> No, I mean tracks that are either kind=subtitles or kind=captions. See for
> instance TextTrack::isRendered(). Adding a new method "isVisual" or
> "isRenderable" or something like that might make sense, and then you could use
> that here.
> 
> Checking LOADED would not be appropriate, but it does add additional
> complication since loading a track could fail, so tracks with readinessState()
> == FailedToLoad should probably be excluded as well.

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:63: static const char*
trackIndexAttr = "x-webkit-track-index";
On 2015/04/17 11:54:46, fs wrote:
> On 2015/04/16 23:37:19, srivats wrote:
> > On 2015/04/14 12:34:56, fs wrote:
> > > const char foo[] = ...
> > > 
> > > Also since this should be in a (closed) shadow tree it seems it could be
> data-
> > > or something instead.
> > 
> > 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?
> 
> 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-.

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:449:
RefPtrWillBeRawPtr<Element> trackItem = doc->createElement(labelTag, false);
On 2015/04/17 11:54:46, fs wrote:
> On 2015/04/16 23:37:20, srivats wrote:
> > On 2015/04/14 12:34:56, fs wrote:
> > > You can call the create() functions directly instead of doing this.
> > 
> > Calling Element::create() doesn't create an HTML element and hence results
in
> > ASSERTION FAILED: element.isStyledElement()
> 
> I believe for this call it'd be HTMLLabelElement::create(), and similarly for
> other HTML elements.

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:462: if (trackIndex >= 0 &&
isShowing)
On 2015/04/17 11:54:46, fs wrote:
> On 2015/04/16 23:37:19, srivats wrote:
> > On 2015/04/14 12:34:57, fs wrote:
> > > Looks like this means multiple tracks can be selected - yet the method
> > enabling
> > > tracks disable all other tracks?
> > 
> > Since the track list is a list of input radio, it's possible to select only
> one
> > track.
> 
> 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.

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.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:489:
tracksHeader->appendChild(doc->createTextNode(mediaElement().locale().queryString(WebLocalizedString::TextTracksSubtitlesCC)));
On 2015/04/17 11:54:46, fs wrote:
> On 2015/04/16 23:37:19, srivats wrote:
> > On 2015/04/14 12:34:56, fs wrote:
> > > What's the 'CC' suffix for - seems redundant unless it should be
> > > SubtitlesAndCaptions or something like that.
> > 
> > I wasn't sure what the header should say. Safari uses just Subtitles and
> YouTube
> > uses Subtitles/CC so went with this.
> 
> 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.

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"

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:501:
createTextTrackListItem(getTextTrackLabel(track), i, track->mode() ==
TextTrack::showingKeyword());
On 2015/04/17 11:54:46, fs wrote:
> On 2015/04/16 23:37:19, srivats wrote:
> > On 2015/04/14 12:34:56, fs wrote:
> > > 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.
> > Passing in TextTrack won't help with reusing this method for creating the
> "Off"
> > item.
> 
> I guess that's a fair point to make, although I guess that could easily be
> represented using 'nullptr'.
> 
> > By non-rendered tracks, are you referring to the tracks that don't have a
> > ready state of LOADED?
> 
> See the similar comment elsewhere.

Done.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControls.cpp:376: // Media controls panel has
overflow set to hidden. To display track list,
On 2015/04/17 11:54:46, fs wrote:
> On 2015/04/16 23:37:20, srivats wrote:
> > On 2015/04/14 12:34:57, fs wrote:
> > > Maybe this is an indication that the menu should not be within the panel
> then?
> > 
> > Seems logical to be a part of media controls because the CC button brings
this
> > menu up. Do you have an alternate approach for this?
> 
> Well, first, a menu that's part of the page has a few issue in that it may end
> up being clipped by the viewport... (compare for instance with <select>)
> 
> Ignoring that though, it should be possible to stack the text track menu on
top
> of the panel, since its layout is independent (AFAICS). Therë́'s already lots
of
> other "overlays" present in the control shadow tree.

Moved it out of media controls panel and added it to the enclosure instead.

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:213: void
setAutomaticTextTrackSelection(bool);
On 2015/04/17 12:06:25, fs wrote:
> On 2015/04/17 11:54:46, fs wrote:
> > This should probably have an "Enabled" suffix to read ok with the bool type.
> 
> With the below, I think this could just be:
> 
> void disableAutomaticTextTrackSelection()

Done.

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:567: bool
m_performAutomaticTextTrackSelection : 1;
On 2015/04/17 12:06:25, fs wrote:
> On 2015/04/17 11:54:46, fs wrote:
> > Move this up to m_closedCaptionsVisible or the other group of related flags.
> 
> I think we should take the opportunity, and introduce the similar flag from
the
> spec:
> 
> https://html.spec.whatwg.org/#did-perform-automatic-track-selection
> 
> So, append 'did' to the name and reverse the value.

Done.

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElementTypes.cpp (right):

https://codereview.chromium.org/1082533002/diff/20001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElementTypes.cpp:95: return
!propertySet->hasProperty(CSSPropertyDisplay);
On 2015/04/17 11:54:46, fs wrote:
> Still needs the null-check I believe.

Done.

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

Starting to look pretty ok, let's try to summon philipj.

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:489:
tracksHeader->appendChild(doc->createTextNode(mediaElement().locale().queryString(WebLocalizedString::TextTracksSubtitlesCC)));
On 2015/04/21 01:48:55, srivats wrote:
> On 2015/04/17 11:54:46, fs wrote:
> > On 2015/04/16 23:37:19, srivats wrote:
> > > On 2015/04/14 12:34:56, fs wrote:
> > > > What's the 'CC' suffix for - seems redundant unless it should be
> > > > SubtitlesAndCaptions or something like that.
> > > 
> > > I wasn't sure what the header should say. Safari uses just Subtitles and
> > YouTube
> > > uses Subtitles/CC so went with this.
> > 
> > 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.
> 
> 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"

Sounds OK to me.

https://codereview.chromium.org/1082533002/diff/40001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/40001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:381:
element->setShadowPseudoId(AtomicString("-webkit-media-controls-text-track-list",
s/webkit/internal/ for all new pseudo elements IIRC.

https://codereview.chromium.org/1082533002/diff/40001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:397: if (!validIndex)
This seems "unexpected", so maybe it should suffice to assert it.

https://codereview.chromium.org/1082533002/diff/40001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:417: if (track->mode() ==
TextTrack::showingKeyword())
Should check isRenderable() here too I think. Might make sense to just merge the
"disabling" and the "enabling".

if (!track->isRenderable())
    continue;
if (i == indexToEnable) {
    ...
} else if (track->mode() == showingKeyword()) {
    ...
}

or somesuch.

https://codereview.chromium.org/1082533002/diff/40001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:470:
trackItem->appendChild(document.createTextNode(getTextTrackLabel(track)));
document.createTextNode(...) -> Text::create(document, ...)

https://codereview.chromium.org/1082533002/diff/40001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:485:
RefPtrWillBeRawPtr<HTMLElement> tracksHeader = HTMLElement::create(h3Tag,
document);
HTMLElement -> HTMLHeadingElement

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

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/1/Source/core/html/shadow/Med...
Source/core/html/shadow/MediaControlElements.cpp:489:
tracksHeader->appendChild(doc->createTextNode(mediaElement().locale().queryString(WebLocalizedString::TextTracksSubtitlesCC)));
On 2015/04/21 12:13:34, fs wrote:
> On 2015/04/21 01:48:55, srivats wrote:
> > On 2015/04/17 11:54:46, fs wrote:
> > > On 2015/04/16 23:37:19, srivats wrote:
> > > > On 2015/04/14 12:34:56, fs wrote:
> > > > > What's the 'CC' suffix for - seems redundant unless it should be
> > > > > SubtitlesAndCaptions or something like that.
> > > > 
> > > > I wasn't sure what the header should say. Safari uses just Subtitles and
> > > YouTube
> > > > uses Subtitles/CC so went with this.
> > > 
> > > 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.
> > 
> > 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"
> 
> Sounds OK to me.

Done.

https://codereview.chromium.org/1082533002/diff/40001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/40001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:381:
element->setShadowPseudoId(AtomicString("-webkit-media-controls-text-track-list",
On 2015/04/21 12:13:34, fs wrote:
> s/webkit/internal/ for all new pseudo elements IIRC.

Done.

https://codereview.chromium.org/1082533002/diff/40001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:397: if (!validIndex)
On 2015/04/21 12:13:34, fs wrote:
> This seems "unexpected", so maybe it should suffice to assert it.

Done.

https://codereview.chromium.org/1082533002/diff/40001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:417: if (track->mode() ==
TextTrack::showingKeyword())
On 2015/04/21 12:13:34, fs wrote:
> Should check isRenderable() here too I think. Might make sense to just merge
the
> "disabling" and the "enabling".
> 
> if (!track->isRenderable())
>     continue;
> if (i == indexToEnable) {
>     ...
> } else if (track->mode() == showingKeyword()) {
>     ...
> }
> 
> or somesuch.

Done.

https://codereview.chromium.org/1082533002/diff/40001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:470:
trackItem->appendChild(document.createTextNode(getTextTrackLabel(track)));
On 2015/04/21 12:13:34, fs wrote:
> document.createTextNode(...) -> Text::create(document, ...)

Done.

https://codereview.chromium.org/1082533002/diff/40001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:485:
RefPtrWillBeRawPtr<HTMLElement> tracksHeader = HTMLElement::create(h3Tag,
document);
On 2015/04/21 12:13:34, fs wrote:
> HTMLElement -> HTMLHeadingElement

Done.

[srivats, 2015-04-27 23:49:25]

Hey Philip, Can you take a look?

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

This reviewed looked big a time-consuming, so I put it off for a while after
getting back to work. Sorry about that.

Overall I'm very excited to get this functionality, the current controls are
just terrible. Lots of details to discuss.

At a higher level, I think we need to involve some UI people. This will change
the behavior of Chrome (and Opera) on all platforms. I could figure out who to
ask at Opera, but can't speak for Google. Unless you know who to involve in this
kind of discussion, maybe ask Dimitri Glazkov what the usual procedure is?

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
File Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:351: bottom: 35px;
You will need to also tweak mediaControlsAndroid.css so that any numbers like
this are correct for the larger size of the controls on Android.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:353: background: rgba(20, 20, 20, 0.8);
Does background:inherit work, given that the parent is the
-webkit-media-controls-panel?

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:355: max-height: 250px;
This means that the list can be bigger than the video and that is in fact what
happens. Oddly enough, it seems like overflow:hidden on the video element has no
effect. Why is that?

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:361: font-family: Arial, Helvetica,
sans-serif;
Since font-family is inherited, can this be moved to
audio::-webkit-media-controls, video::-webkit-media-controls { ... }?

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:363: display: block;
Is display:block needed given that this is already a div element?

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:366: font-size: 16px;
I found it a bit jarring that the font size is so much bigger than the the time
display in the controls.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:373: color: gray;
I found this rather low contrast, probably not readable enough for all people.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:395: top: -2px;
What's this for? Is the image element misaligned (vertical-align:middle not
working) or is this compensating for the PNG itself not having the checkmark
where it should?

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:549: bool
m_didPerformAutomaticTrackSelection : 1;
This name is a bit inaccurate since it can be set to one without actually
performing automatic text track selection. Maybe shouldPerform... would be
better.

However, as I say in another comment, I'm not sure this code is needed at all.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElementTypes.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElementTypes.cpp:92: bool
MediaControlElement::isVisible()
I've been meaning to change hide() and show() to use the hidden attribute
instead, partly because of the rumor that this is a stronger signal that
display:none that it should be ignored by screen readers. Can you name this
isHidden() instead so that it would make sense if that change is made? (It also
avoids the impression that this could have to do with the visibility attribute.)

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:65: // track index attribute
specifies trackIndex for text track list elements
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.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:66: static const char
trackIndexAttr[] = "data-webkit-track-index";
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.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:68: // when specified as
trackIndex, disable text tracks
Is a special value really needed for this, couldn't the absence of the attribute
be used instead?

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:401:
showTextTrackAtIndex(trackIndex);
What happens if the text tracks change while this menu is showing? It looks like
the wrong track will be enabled?

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:402:
mediaElement().setClosedCaptionsVisible(true);
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.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:418: if
(!track->isRenderable())
Is this really needed?

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:420: if (i == indexToEnable &&
indexToEnable < trackList->length()) {
The indexToEnable < trackList->length() check could be moved out of the loop.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:422:
mediaElement().disableAutomaticTextTrackSelection();
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.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:437: trackLabel =
track->language();
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).

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:442: // When the track kind is
captions, add a captions marker to distinguish between captions and subtitles.
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.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:448:
RefPtrWillBeRawPtr<Element>
MediaControlTextTrackListElement::createTextTrackListItem(TextTrack* track)
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?

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:464: if (track && track->mode()
== TextTrack::showingKeyword())
If there are multiple tracks showing, I guess they will all have a checkmark?
Perhaps document this to make it explicit.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:489:
mediaElement().locale().queryString(WebLocalizedString::TextTracksSubtitles)));
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.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:492:
tracksSection->setAttribute(aria_labeledbyAttr, tracksSectionId);
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.)

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:494:
tracksSection->appendChild(createTextTrackListItem(nullptr));
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.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:99: //    
+-MediaControlTextTrackListElement             
(-internal-media-controls-text-track-list)
Can you also document what the internal structure of this looks like, even if it
isn't always created?

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/track/...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/track/...
Source/core/html/track/TextTrack.cpp:463: // A track can be displayed when it's
of kind captions or subtitles and hasn't failed to load
I guess this means that if the readinessState is NotLoaded and one tries to
enable the track, the next time one opens the menu that track will simply be
gone?

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

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
File Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:355: max-height: 250px;
On 2015/05/05 14:36:56, philipj_UTC2 wrote:
> This means that the list can be bigger than the video and that is in fact what
> happens. Oddly enough, it seems like overflow:hidden on the video element has
no
> effect. Why is that?

Probably because the video element uses LayoutReplaced (which doesn't handle
overflow - because it [usually] never overflows IIUC)

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:549: bool
m_didPerformAutomaticTrackSelection : 1;
On 2015/05/05 14:36:56, philipj_UTC2 wrote:
> This name is a bit inaccurate since it can be set to one without actually
> performing automatic text track selection. Maybe shouldPerform... would be
> better.
> 
> However, as I say in another comment, I'm not sure this code is needed at all.

I believe I suggested this name, with an outset of implementing the flag from
the spec with the same name (only dash-separated of course...) I agree with your
other comment though.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:402:
mediaElement().setClosedCaptionsVisible(true);
On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> 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.

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.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:418: if
(!track->isRenderable())
On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> Is this really needed?

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

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:437: trackLabel =
track->language();
On 2015/05/05 14:36:56, philipj_UTC2 wrote:
> 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).

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

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:442: // When the track kind is
captions, add a captions marker to distinguish between captions and subtitles.
On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> 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.

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

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:448:
RefPtrWillBeRawPtr<Element>
MediaControlTextTrackListElement::createTextTrackListItem(TextTrack* track)
On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> 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?

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.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:492:
tracksSection->setAttribute(aria_labeledbyAttr, tracksSectionId);
On 2015/05/05 14:36:56, philipj_UTC2 wrote:
> 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.)

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

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/track/...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/track/...
Source/core/html/track/TextTrack.cpp:463: // A track can be displayed when it's
of kind captions or subtitles and hasn't failed to load
On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> I guess this means that if the readinessState is NotLoaded and one tries to
> enable the track, the next time one opens the menu that track will simply be
> gone?

Yes. Optionally I suppose one could "gray it out" or similar (it's pretty
annoying with things you can interact with but which doesn't do anything...) UI
issue really though.

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

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
File Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:355: max-height: 250px;
On 2015/05/05 15:32:44, fs wrote:
> On 2015/05/05 14:36:56, philipj_UTC2 wrote:
> > This means that the list can be bigger than the video and that is in fact
what
> > happens. Oddly enough, it seems like overflow:hidden on the video element
has
> no
> > effect. Why is that?
> 
> Probably because the video element uses LayoutReplaced (which doesn't handle
> overflow - because it [usually] never overflows IIUC)

Oh right. I tried display:block too but in hindsight that couldn't make the
video anything other than LayoutReplaced because if it's not a LayoutMedia the
controls wouldn't even be visible. Anyway, I think it's just fine that
overflow:hidden doesn't work, just wanted to understand why.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.h:549: bool
m_didPerformAutomaticTrackSelection : 1;
On 2015/05/05 15:32:44, fs wrote:
> On 2015/05/05 14:36:56, philipj_UTC2 wrote:
> > This name is a bit inaccurate since it can be set to one without actually
> > performing automatic text track selection. Maybe shouldPerform... would be
> > better.
> > 
> > However, as I say in another comment, I'm not sure this code is needed at
all.
> 
> I believe I suggested this name, with an outset of implementing the flag from
> the spec with the same name (only dash-separated of course...) I agree with
your
> other comment though.

Oh, I didn't know about
https://html.spec.whatwg.org/#did-perform-automatic-track-selection or at least
forgot. We should implement that, but in the precise places where the spec says.
With some luck, it might not be needed in this CL at all.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:402:
mediaElement().setClosedCaptionsVisible(true);
On 2015/05/05 15:32:44, fs wrote:
> On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> > 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.
> 
> 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.

Ahem, will get to that review too...

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:448:
RefPtrWillBeRawPtr<Element>
MediaControlTextTrackListElement::createTextTrackListItem(TextTrack* track)
On 2015/05/05 15:32:44, fs wrote:
> On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> > 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?
> 
> 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.

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

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:492:
tracksSection->setAttribute(aria_labeledbyAttr, tracksSectionId);
On 2015/05/05 15:32:44, fs wrote:
> On 2015/05/05 14:36:56, philipj_UTC2 wrote:
> > 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.)
> 
> I think it would be weird if it wasn't scoped...

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, 2015-06-03 22:30:40]

On 2015/05/06 09:28:43, philipj wrote:
>
https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
> File Source/core/css/mediaControls.css (right):
> 
>
https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
> Source/core/css/mediaControls.css:355: max-height: 250px;
> On 2015/05/05 15:32:44, fs wrote:
> > On 2015/05/05 14:36:56, philipj_UTC2 wrote:
> > > This means that the list can be bigger than the video and that is in fact
> what
> > > happens. Oddly enough, it seems like overflow:hidden on the video element
> has
> > no
> > > effect. Why is that?
> > 
> > Probably because the video element uses LayoutReplaced (which doesn't handle
> > overflow - because it [usually] never overflows IIUC)
> 
> Oh right. I tried display:block too but in hindsight that couldn't make the
> video anything other than LayoutReplaced because if it's not a LayoutMedia the
> controls wouldn't even be visible. Anyway, I think it's just fine that
> overflow:hidden doesn't work, just wanted to understand why.
> 
>
https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/HTMLMe...
> File Source/core/html/HTMLMediaElement.h (right):
> 
>
https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/HTMLMe...
> Source/core/html/HTMLMediaElement.h:549: bool
> m_didPerformAutomaticTrackSelection : 1;
> On 2015/05/05 15:32:44, fs wrote:
> > On 2015/05/05 14:36:56, philipj_UTC2 wrote:
> > > This name is a bit inaccurate since it can be set to one without actually
> > > performing automatic text track selection. Maybe shouldPerform... would be
> > > better.
> > > 
> > > However, as I say in another comment, I'm not sure this code is needed at
> all.
> > 
> > I believe I suggested this name, with an outset of implementing the flag
from
> > the spec with the same name (only dash-separated of course...) I agree with
> your
> > other comment though.
> 
> Oh, I didn't know about
> https://html.spec.whatwg.org/#did-perform-automatic-track-selection or at
least
> forgot. We should implement that, but in the precise places where the spec
says.
> With some luck, it might not be needed in this CL at all.
> 
>
https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
> File Source/core/html/shadow/MediaControlElements.cpp (right):
> 
>
https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
> Source/core/html/shadow/MediaControlElements.cpp:402:
> mediaElement().setClosedCaptionsVisible(true);
> On 2015/05/05 15:32:44, fs wrote:
> > On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> > > 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.
> > 
> > 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.
> 
> Ahem, will get to that review too...
> 
>
https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
> Source/core/html/shadow/MediaControlElements.cpp:448:
> RefPtrWillBeRawPtr<Element>
> MediaControlTextTrackListElement::createTextTrackListItem(TextTrack* track)
> On 2015/05/05 15:32:44, fs wrote:
> > On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> > > 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?
> > 
> > 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.
> 
> Sorry, I skipped the history and comments, so we're touching on the same areas
> with opposing suggestions :)
> 
>
https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
> Source/core/html/shadow/MediaControlElements.cpp:492:
> tracksSection->setAttribute(aria_labeledbyAttr, tracksSectionId);
> On 2015/05/05 15:32:44, fs wrote:
> > On 2015/05/05 14:36:56, philipj_UTC2 wrote:
> > > 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.)
> > 
> > I think it would be weird if it wasn't scoped...
> 
> 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.


FYI:
Here's the issue for track selection UX: crbug.com/495851

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

I've responded to the comments here.

I had to merge this code review with https://codereview.chromium.org/1079323002/
because of the Blink merge.
All further changes will be made there.

Thanks,
Sunitha

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
File Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:351: bottom: 35px;
On 2015/05/05 14:36:56, philipj_UTC7 wrote:
> You will need to also tweak mediaControlsAndroid.css so that any numbers like
> this are correct for the larger size of the controls on Android.

Will do.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:353: background: rgba(20, 20, 20, 0.8);
On 2015/05/05 14:36:56, philipj wrote:
> Does background:inherit work, given that the parent is the
> -webkit-media-controls-panel?

The parent is now the enclosure so background:inherit won't work.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:361: font-family: Arial, Helvetica,
sans-serif;
On 2015/05/05 14:36:56, philipj wrote:
> Since font-family is inherited, can this be moved to
> audio::-webkit-media-controls, video::-webkit-media-controls { ... }?

Done.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:363: display: block;
On 2015/05/05 14:36:56, philipj wrote:
> Is display:block needed given that this is already a div element?

Removed

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:366: font-size: 16px;
On 2015/05/05 14:36:56, philipj wrote:
> I found it a bit jarring that the font size is so much bigger than the the
time
> display in the controls.

Made it the same as the font-size of time display.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:373: color: gray;
On 2015/05/05 14:36:56, philipj wrote:
> I found this rather low contrast, probably not readable enough for all people.

Changed it here and in the new material design UI.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:395: top: -2px;
On 2015/05/05 14:36:56, philipj wrote:
> What's this for? Is the image element misaligned (vertical-align:middle not
> working) or is this compensating for the PNG itself not having the checkmark
> where it should?

It looks misaligned without the -2px. Might be an issue with the PNG but I tried
using a different one and it still looked off.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElementTypes.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElementTypes.cpp:92: bool
MediaControlElement::isVisible()
On 2015/05/05 14:36:56, philipj wrote:
> I've been meaning to change hide() and show() to use the hidden attribute
> instead, partly because of the rumor that this is a stronger signal that
> display:none that it should be ignored by screen readers. Can you name this
> isHidden() instead so that it would make sense if that change is made? (It
also
> avoids the impression that this could have to do with the visibility
attribute.)

Done.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:65: // track index attribute
specifies trackIndex for text track list elements
On 2015/05/05 14:36:57, philipj wrote:
> 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.

Done.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:66: static const char
trackIndexAttr[] = "data-webkit-track-index";
On 2015/05/05 14:36:57, philipj wrote:
> 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.

Done.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:68: // when specified as
trackIndex, disable text tracks
On 2015/05/05 14:36:57, philipj_OOO_til_Feb15 wrote:
> Is a special value really needed for this, couldn't the absence of the
attribute
> be used instead?

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

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:401:
showTextTrackAtIndex(trackIndex);
On 2015/05/05 14:36:56, philipj_UTC7 wrote:
> What happens if the text tracks change while this menu is showing? It looks
like
> the wrong track will be enabled?

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?

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:402:
mediaElement().setClosedCaptionsVisible(true);
On 2015/05/06 09:28:43, philipj_UTC7 wrote:
> On 2015/05/05 15:32:44, fs wrote:
> > On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> > > 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.
> > 
> > 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.
> 
> Ahem, will get to that review too...
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.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:420: if (i == indexToEnable &&
indexToEnable < trackList->length()) {
On 2015/05/05 14:36:57, philipj wrote:
> The indexToEnable < trackList->length() check could be moved out of the loop.

Done.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:442: // When the track kind is
captions, add a captions marker to distinguish between captions and subtitles.
On 2015/05/05 15:32:44, fs wrote:
> On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> > 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.
> 
> Since it is localized, it could well be "" for Swedish, "(HoH)" for en-GB etc.

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

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:448:
RefPtrWillBeRawPtr<Element>
MediaControlTextTrackListElement::createTextTrackListItem(TextTrack* track)
On 2015/05/06 09:28:43, philipj wrote:
> On 2015/05/05 15:32:44, fs wrote:
> > On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> > > 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?
> > 
> > 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.
> 
> Sorry, I skipped the history and comments, so we're touching on the same areas
> with opposing suggestions :)

Added a comment explaining what the nullptr is for.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:464: if (track && track->mode()
== TextTrack::showingKeyword())
On 2015/05/05 14:36:57, philipj wrote:
> If there are multiple tracks showing, I guess they will all have a checkmark?
> Perhaps document this to make it explicit.

Yes. I tested this case out.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:489:
mediaElement().locale().queryString(WebLocalizedString::TextTracksSubtitles)));
On 2015/05/05 14:36:57, philipj_UTC7 wrote:
> 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.

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

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:492:
tracksSection->setAttribute(aria_labeledbyAttr, tracksSectionId);
On 2015/05/06 09:28:43, philipj_UTC7 wrote:
> On 2015/05/05 15:32:44, fs wrote:
> > On 2015/05/05 14:36:56, philipj_UTC2 wrote:
> > > 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.)
> > 
> > I think it would be weird if it wasn't scoped...
> 
> 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.

No more header.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:494:
tracksSection->appendChild(createTextTrackListItem(nullptr));
On 2015/05/05 14:36:57, philipj_UTC7 wrote:
> 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.

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.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControls.cpp:99: //    
+-MediaControlTextTrackListElement             
(-internal-media-controls-text-track-list)
On 2015/05/05 14:36:57, philipj_OOO_til_Feb15 wrote:
> Can you also document what the internal structure of this looks like, even if
it
> isn't always created?

Done

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/track/...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/track/...
Source/core/html/track/TextTrack.cpp:463: // A track can be displayed when it's
of kind captions or subtitles and hasn't failed to load
On 2015/05/05 15:32:44, fs wrote:
> On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> > I guess this means that if the readinessState is NotLoaded and one tries to
> > enable the track, the next time one opens the menu that track will simply be
> > gone?
> 
> Yes. Optionally I suppose one could "gray it out" or similar (it's pretty
> annoying with things you can interact with but which doesn't do anything...)
UI
> issue really though.
Should we just exclude the NotLoaded tracks from being displayed?

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

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
File Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
Source/core/css/mediaControls.css:395: top: -2px;
On 2016/02/23 01:39:26, srivats wrote:
> On 2015/05/05 14:36:56, philipj wrote:
> > What's this for? Is the image element misaligned (vertical-align:middle not
> > working) or is this compensating for the PNG itself not having the checkmark
> > where it should?
> 
> It looks misaligned without the -2px. Might be an issue with the PNG but I
tried
> using a different one and it still looked off.

Following up in other CL.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElementTypes.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElementTypes.cpp:92: bool
MediaControlElement::isVisible()
On 2016/02/23 01:39:26, srivats wrote:
> On 2015/05/05 14:36:56, philipj wrote:
> > I've been meaning to change hide() and show() to use the hidden attribute
> > instead, partly because of the rumor that this is a stronger signal that
> > display:none that it should be ignored by screen readers. Can you name this
> > isHidden() instead so that it would make sense if that change is made? (It
> also
> > avoids the impression that this could have to do with the visibility
> attribute.)
> 
> Done.

Well this changed again since last, sorry about that. (See other CL.)

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:401:
showTextTrackAtIndex(trackIndex);
On 2016/02/23 01:39:27, srivats wrote:
> On 2015/05/05 14:36:56, philipj_UTC7 wrote:
> > What happens if the text tracks change while this menu is showing? It looks
> like
> > the wrong track will be enabled?
> 
> 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?

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.

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
Source/core/html/shadow/MediaControlElements.cpp:442: // When the track kind is
captions, add a captions marker to distinguish between captions and subtitles.
On 2016/02/23 01:39:27, srivats wrote:
> On 2015/05/05 15:32:44, fs wrote:
> > On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> > > 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.
> > 
> > Since it is localized, it could well be "" for Swedish, "(HoH)" for en-GB
etc.
> 
> UX suggested I use icons here to differentiate between the two track kinds

And I really like that new subtitles icon!

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/track/...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/track/...
Source/core/html/track/TextTrack.cpp:463: // A track can be displayed when it's
of kind captions or subtitles and hasn't failed to load
On 2016/02/23 01:39:27, srivats wrote:
> On 2015/05/05 15:32:44, fs wrote:
> > On 2015/05/05 14:36:57, philipj_UTC2 wrote:
> > > I guess this means that if the readinessState is NotLoaded and one tries
to
> > > enable the track, the next time one opens the menu that track will simply
be
> > > gone?
> > 
> > Yes. Optionally I suppose one could "gray it out" or similar (it's pretty
> > annoying with things you can interact with but which doesn't do anything...)
> UI
> > issue really though.
> Should we just exclude the NotLoaded tracks from being displayed?

I think what you have now in the other CL seems to work well. We can't exclude
NotLoaded tracks, because that could be all of them!