Bug 106285

On a second thought, I think showing a button in a disabled state would be better. This would indicate that <track> elements are present and are supposed to be shown, but something went wrong. Clicking on it in that disabled state should try reloading the tracks. Would this make sense?

Sounds good to me.

(In reply to comment #1) > On a second thought, I think showing a button in a disabled state would be > better. This would indicate that <track> elements are present and are supposed > to be shown, but something went wrong. > > Clicking on it in that disabled state should try reloading the tracks. > Would this make sense? In what circumstances would reloading work? How is a user supposed to know that clicking a disabled button would have this action?

(In reply to comment #3) > (In reply to comment #1) > > On a second thought, I think showing a button in a disabled state would be > > better. This would indicate that <track> elements are present and are supposed > > to be shown, but something went wrong. > > > > Clicking on it in that disabled state should try reloading the tracks. > > Would this make sense? > > In what circumstances would reloading work? How is a user supposed to know that clicking a disabled button would have this action? What happens right now if a track wasn't loaded because the machine serving it is down, or there are communication issues? Does it retry indefinitely? I was thinking at these cases particularly; the track failed loading, but then the machine serving it becomes available and the user can force a retry toggling the disabled button.

(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #1) > > > On a second thought, I think showing a button in a disabled state would be > > > better. This would indicate that <track> elements are present and are supposed > > > to be shown, but something went wrong. > > > > > > Clicking on it in that disabled state should try reloading the tracks. > > > Would this make sense? > > > > In what circumstances would reloading work? How is a user supposed to know that clicking a disabled button would have this action? > > What happens right now if a track wasn't loaded because the machine serving > it is down, or there are communication issues? Does it retry indefinitely? > The same thing that happens when any other resource (eg. image, script, etc) doesn't load - nothing. > I was thinking at these cases particularly; the track failed loading, but > then the machine serving it becomes available and the user can force a retry > toggling the disabled button. > I think just reloading the page makes sense, that way all resources from the same server will reload.

Hmm I think I agree with Eric - you can always hit the browser "reload" button to get the captions. YouTube doesn't do a reload either. Also note that the resources in a list of <track> elements may not all come from the same server. If they all don't load, it's probably not necessary to display a button.

Created attachment 185287 [details] Patch Updated behavior in several places

Comment on attachment 185287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185287&action=review > Source/WebCore/html/HTMLMediaElement.cpp:1336 > + if (m_textTracksWhenResourceSelectionBegan[i]->readinessState() == TextTrack::Loading > + || m_textTracksWhenResourceSelectionBegan[i]->readinessState() == TextTrack::NotLoaded) > return false; Won't this allow m_readyState to get stuck at HAVE_CURRENT_DATA if a track fails to load?

(In reply to comment #8) > (From update of attachment 185287 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185287&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:1336 > > + if (m_textTracksWhenResourceSelectionBegan[i]->readinessState() == TextTrack::Loading > > + || m_textTracksWhenResourceSelectionBegan[i]->readinessState() == TextTrack::NotLoaded) > > return false; > > Won't this allow m_readyState to get stuck at HAVE_CURRENT_DATA if a track fails to load? I will look closer in the code, but do you have a specific case in which you would expect to fail (i.e., visually / behavior?)

(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 185287 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=185287&action=review > > > > > Source/WebCore/html/HTMLMediaElement.cpp:1336 > > > + if (m_textTracksWhenResourceSelectionBegan[i]->readinessState() == TextTrack::Loading > > > + || m_textTracksWhenResourceSelectionBegan[i]->readinessState() == TextTrack::NotLoaded) > > > return false; > > > > Won't this allow m_readyState to get stuck at HAVE_CURRENT_DATA if a track fails to load? > > I will look closer in the code, but do you have a specific case in which you > would expect to fail (i.e., visually / behavior?) HTMLMediaElement::mediaPlayerReadyStateChanged has this: if (tracksAreReady) m_readyState = newState; else { // If a media file has text tracks the readyState may not progress beyond HAVE_FUTURE_DATA until // the text tracks are ready, regardless of the state of the media file. if (newState <= HAVE_METADATA) m_readyState = newState; else m_readyState = HAVE_CURRENT_DATA; } So I believe that a track that never loads, eg. one that has a bogus url, will prevent the element's m_readyState from advancing beyond HAVE_CURRENT_DATA.

(In reply to comment #10) > So I believe that a track that never loads, eg. one that has a bogus url, will prevent the element's m_readyState from advancing beyond HAVE_CURRENT_DATA. I think we're safe: if it fails to load, the readinessState changes to FailedToLoad and doesn't remain NotLoaded (as this is the check I added) It's following the spec comment from ::textTracksAreReady - "[...] have a text track readiness state of loaded or failed to load." so it should be fine. I manually tested, but I could add the case in a layout test as well if you prefer.

(In reply to comment #11) > (In reply to comment #10) > > So I believe that a track that never loads, eg. one that has a bogus url, will prevent the element's m_readyState from advancing beyond HAVE_CURRENT_DATA. > > I think we're safe: if it fails to load, the readinessState changes to FailedToLoad and doesn't remain NotLoaded (as this is the check I added) > > It's following the spec comment from ::textTracksAreReady - "[...] have a text track readiness state of loaded or failed to load." so it should be fine. > You are correct, of course. > I manually tested, but I could add the case in a layout test as well if you > prefer. > I think this is already covered by the existing tests, if it actually was a problem a <video> with an unload able <track> would never reach canplaythrough.

Created attachment 185897 [details] Extra method Added extra method for refreshing the cc button visibility

<rdar://problem/13131121>

Comment on attachment 185897 [details] Extra method Clearing flags on attachment: 185897 Committed r141531: <http://trac.webkit.org/changeset/141531>

All reviewed patches have been landed. Closing bug.