Bug 168168 – REGRESSION: Subtitles menu in media controls allows multiple items to be selected

• Component: Media
• Status: RESOLVED FIXED

To reproduce this bug, navigate to a video URL. Click the Subtitles button in the controls to bring up the menu. Click an item that isn’t selected, then click the Subtitles button again, and notice how the item is now selected, but the previously-selected item is selected too. Notice that you can toggle the selected state of each item independently of the others.

[Antoine Quint]

Indeed, this isn't as expected. I was mistakenly thinking we should support this since the media API supports it, but this is not what we want to do from a UI perspective.

[Radar WebKit Bug Importer]

<rdar://problem/30488605>

[Antoine Quint]

Created attachment 301924 [details]
Patch

[Dean Jackson]

Comment on attachment 301924 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301924&action=review

> Source/WebCore/Modules/modern-media-controls/media/tracks-support.js:109
> +            this._audioTracks().forEach((audioTrack, index) => audioTrack.enabled = index === trackIndex);

I'm not sure if this is allowed in WebKit style, but I'd appreciate () around the equality test. Err... maybe not. "Whatever, dude. Whatever."

> LayoutTests/media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html:4
> +<video src="../../content/CC+Subtitles.mov" style="position: absolute; left: 0; top: 0; width: 640px; height: 360px;" controls autoplay></video>

Why did you have to do this?

> LayoutTests/media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html:62
> +    const newSelectedTrack = Array.from(media.audioTracks).findIndex(track => track.enabled);

Wow. I'd either forgotten or never heard of findIndex.

[Antoine Quint]

(In reply to comment #4)
> Comment on attachment 301924 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301924&action=review
> 
> > Source/WebCore/Modules/modern-media-controls/media/tracks-support.js:109
> > +            this._audioTracks().forEach((audioTrack, index) => audioTrack.enabled = index === trackIndex);
> 
> I'm not sure if this is allowed in WebKit style, but I'd appreciate ()
> around the equality test. Err... maybe not. "Whatever, dude. Whatever."

I guess I'll just leave it as-is then.

> > LayoutTests/media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html:4
> > +<video src="../../content/CC+Subtitles.mov" style="position: absolute; left: 0; top: 0; width: 640px; height: 360px;" controls autoplay></video>
> 
> Why did you have to do this?

I think this made obtaining metrics more predictable as logs were added to the document.

> > LayoutTests/media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html:62
> > +    const newSelectedTrack = Array.from(media.audioTracks).findIndex(track => track.enabled);
> 
> Wow. I'd either forgotten or never heard of findIndex.

I implemented it 3 years ago, my only call to fame in the JSC world :)

[Antoine Quint]

Committed r212571: <http://trac.webkit.org/changeset/212571>