Bug 169675

Summary: [Modern Media Controls] Captions do not default to Auto when language is changed
Product: WebKit Reporter: Antoine Quint <graouts>
Component: MediaAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Antoine Quint 2017-03-15 08:02:46 PDT
Steps To Reproduce:
1. Make sure the captions mode is set to “Auto” (open a video in QuickTime Player and select Auto in captions menu)
2. Go to System Preferences -> Language & Region, and change the system language to French
3. Play a video with French captions, and open the captions menu

Results:
The captions menu shows that the French subtitles are selected. It should show that Auto is selected.
Comment 1 Antoine Quint 2017-03-15 08:03:46 PDT
<rdar://problem/30423369>
Comment 2 Antoine Quint 2017-03-15 08:08:22 PDT
Created attachment 304501 [details]
Patch
Comment 3 Eric Carlson 2017-03-15 08:19:44 PDT
Comment on attachment 304501 [details]
Patch

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

as discussed on irc, you should be able to create a test for this with internals.setUserPreferredLanguages().

> Source/WebCore/Modules/modern-media-controls/media/tracks-support.js:115
> +        const trackItem = this._textTracks()[trackIndex];
> +        const host = this.mediaController.host;
> +        const usesAutomaticTrack = host ? host.captionDisplayMode === "automatic" : false;
> +
> +        if (host) {
> +            if (trackItem === host.captionMenuOffItem && (host.captionDisplayMode === "forced-only" || host.captionDisplayMode === "manual"))
> +                return true;
> +            else if (trackItem === host.captionMenuAutomaticItem && usesAutomaticTrack)
> +                return true;
> +        }
> +
> +        return !usesAutomaticTrack && trackItem.mode !== "disabled";

I thinks this would be slightly easier to follow if you return immediately if host is null:

if (!host)
    return trackItem.mode !== "disabled";

if (trackItem === host.captionMenuOffItem && (host.captionDisplayMode === "forced-only" || host.captionDisplayMode === "manual"))
    return true;

if (trackItem === host.captionMenuAutomaticItem && usesAutomaticTrack)
    return true;

return trackItem.mode !== "disabled";
Comment 4 Eric Carlson 2017-03-15 08:21:17 PDT
Comment on attachment 304501 [details]
Patch

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

>> Source/WebCore/Modules/modern-media-controls/media/tracks-support.js:115
>> +        return !usesAutomaticTrack && trackItem.mode !== "disabled";
> 
> I thinks this would be slightly easier to follow if you return immediately if host is null:
> 
> if (!host)
>     return trackItem.mode !== "disabled";
> 
> if (trackItem === host.captionMenuOffItem && (host.captionDisplayMode === "forced-only" || host.captionDisplayMode === "manual"))
>     return true;
> 
> if (trackItem === host.captionMenuAutomaticItem && usesAutomaticTrack)
>     return true;
> 
> return trackItem.mode !== "disabled";

or:

if (host && trackItem === host.captionMenuOffItem && (host.captionDisplayMode === "forced-only" || host.captionDisplayMode === "manual"))
    return true;

if (host && trackItem === host.captionMenuAutomaticItem && usesAutomaticTrack)
    return true;

return trackItem.mode !== "disabled";
Comment 5 Antoine Quint 2017-03-15 09:02:36 PDT
Created attachment 304504 [details]
Patch
Comment 6 Eric Carlson 2017-03-15 10:04:52 PDT
Comment on attachment 304504 [details]
Patch

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

> Source/WebCore/Modules/modern-media-controls/media/tracks-support.js:106
> +        const usesAutomaticTrack = host ? host.captionDisplayMode === "automatic" : false;

Nit: this variable is no longer necessary.
Comment 7 Antoine Quint 2017-03-15 10:11:24 PDT
(In reply to comment #6)
> Comment on attachment 304504 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304504&action=review
> 
> > Source/WebCore/Modules/modern-media-controls/media/tracks-support.js:106
> > +        const usesAutomaticTrack = host ? host.captionDisplayMode === "automatic" : false;
> 
> Nit: this variable is no longer necessary.

It is necessary in case the track that we’re querying the selected state for is neither Off or Auto but we should be selecting auto.
Comment 8 WebKit Commit Bot 2017-03-15 10:41:03 PDT
Comment on attachment 304504 [details]
Patch

Clearing flags on attachment: 304504

Committed r213987: <http://trac.webkit.org/changeset/213987>
Comment 9 WebKit Commit Bot 2017-03-15 10:41:06 PDT
All reviewed patches have been landed.  Closing bug.