Bug 105536 – <track> element's mode set to "disabled" after load although it was explicitly set to "hidden"

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Antoine Quint]

A lot of the Opera-submitted <track> tests added to regression test suite being skipped due to a failure related to the track's mode being set to "disabled" even though it was originally set to "hidden" in script. After debugging, it appears the change of mode is performed in HTMLMediaElement::configureTextTrackGroup() which is called when the <video> element to which the <track> element is attached is loaded. It appears the code in question relates to the concept of a "showing by default" mode which was present in an older spec draft (http://www.w3.org/TR/2011/WD-html5-20110405/video.html#text-track-showing-by-default) but is no longer available. It sounds to me that all concept of "showing by default" should be removed from the codebase, and hopefully remove this issue.

These are the affected tests that pass if the call to textTrack->setMode(TextTrack::disabledKeyword()) in HTMLMediaElement::configureTextTrackGroup() is commented out:

LayoutTests/media/track/opera/interfaces/TextTrack/addCue.html
LayoutTests/media/track/opera/interfaces/TextTrack/removeCue.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/endTime.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/startTime.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/align.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/id.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/pauseOnExit.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/track.html

These tests are also affected but have other failures beyond this specific issue with "showing by default":

LayoutTests/media/track/opera/interfaces/TextTrackCue/snapToLines.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/text.html
LayoutTests/media/track/opera/interfaces/TextTrackCue/vertical.html

[Radar WebKit Bug Importer]

<rdar://problem/12917581>

[Antoine Quint]

Created attachment 180525 [details]
WIP

Attaching work-in-progress patch where we remove all concept of "showing by default". This means removing the showingByDefault() and setShowingByDefault() methods on TextTrack. As I was going through this code, I also noticed two oddities:

1. TextTrack::setIsDefault() didn't do anything and TextTrack::isDefault() always returned false. There is now a new m_isDefault ivar that actually remembers the isDefault status.

2. In HTMLMediaElement::configureTextTrackGroup(), we would set defaultTrack to textTrack.get() even though defaultTrack was created to be a RefPtr<TextTrack>. This just seemed wrong, probably an oversight.

As a result, we can now pass an additional 9 tests from the Opera-submitted test suite.

I'm not submitting this patch for review yet as test results depends on https://bugs.webkit.org/show_bug.cgi?id=103258 and that bug's patch has been rolled back.

[Eric Carlson]

Comment on attachment 180525 [details]
WIP

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

Looks good to me, thank you!

> Source/WebCore/html/HTMLMediaElement.cpp:3011
> -            defaultTrack = textTrack.get();
> +            defaultTrack = textTrack;

Oops, good catch!

> Source/WebCore/html/track/TextTrack.h:121
> +    virtual bool isDefault() const { return m_isDefault; }
> +    virtual void setIsDefault(bool flag) { m_isDefault = flag; }

"default" never applies to tracks created with addTextTrack(), so I would leave the "isDefault() const { return false; }" and remove setIsDefault() and "m_isDefault". LoadableTextTrack is the only track type that needs setIsDefault() so you can make it non-virtual.

[Antoine Quint]

Created attachment 181497 [details]
Patch

[WebKit Review Bot]

Comment on attachment 181497 [details]
Patch

Attachment 181497 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15739777

New failing tests:
media/video-controls-captions.html

[Antoine Quint]

(In reply to comment #5)
> (From update of attachment 181497 [details])
> Attachment 181497 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/15739777
> 
> New failing tests:
> media/video-controls-captions.html

Raised https://bugs.webkit.org/show_bug.cgi?id=106230 to track this issue. Will upload a new patch to expect failure for this test.

[Antoine Quint]

Created attachment 181532 [details]
Patch

[Antoine Quint]

Created attachment 181563 [details]
Updated patch which should apply correctly.

[WebKit Review Bot]

Comment on attachment 181563 [details]
Updated patch which should apply correctly.

Clearing flags on attachment: 181563

Committed r139080: <http://trac.webkit.org/changeset/139080>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.