1332994 - The subtitle menu should highlight current enabled CC when video first loaded

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Ray Lin[:ralin]]

This is the followup of bug 1329117. After bug 1329117 landed, we can get the correct `track.mode` from `addtrack` event and we should update menu accordingly.

Comment 1

[Ray Lin[:ralin]]

Attachment: Bug 1332994 - highlight current enabled CC item when video first loaded.

Review commit: https://reviewboard.mozilla.org/r/106454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106454/

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8829337 [details]
Bug 1332994 - highlight current enabled CC item when video first loaded.

https://reviewboard.mozilla.org/r/106454/#review107588

The change looks find, but this should have a test with it.

Comment 3

[Ray Lin[:ralin]]

Comment on attachment 8829337 [details]
Bug 1332994 - highlight current enabled CC item when video first loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106454/diff/1-2/

Comment 4

[Ray Lin[:ralin]]

Comment on attachment 8829337 [details]
Bug 1332994 - highlight current enabled CC item when video first loaded.

https://reviewboard.mozilla.org/r/106454/#review107588

I added a test case to verify item's status after adding a 'showing' subtitle.  

Thanks :)

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8829337 [details]
Bug 1332994 - highlight current enabled CC item when video first loaded.

https://reviewboard.mozilla.org/r/106454/#review109922

::: toolkit/content/tests/widgets/test_videocontrols_vtt.html:91
(Diff revision 2)
> +    const tt = ttList.children[1];
> +
> +    synthesizeMouseAtCenter(tt, {});
> +
> +    SimpleTest.executeSoon(() => {
> +      is(tt.getAttribute("on"), "true", "Selected item should be enabled");
> +      is(ttList.getAttribute("hidden"), "true", "Should hide texttrack menu once clicked on an item");

Please add a test condition that checks that tt.getAttribute("on") is not equal to "true" before synthesizeMouseAtCenter(tt, {});

Comment 6

[Ray Lin[:ralin]]

Comment on attachment 8829337 [details]
Bug 1332994 - highlight current enabled CC item when video first loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106454/diff/2-3/

Comment 7

[Carsten Book [:Tomcat]]

seems there is one open issue in mozreview that needs be to be fixed before we can use autoland. Can you fix this, thanks!

Comment 8

[Ray Lin[:ralin]]

(In reply to Carsten Book [:Tomcat] from comment #7)
> seems there is one open issue in mozreview that needs be to be fixed before
> we can use autoland. Can you fix this, thanks!

I thought I've clicked on fixed button already... sorry for that. The issue is resolved now, thanks :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c620fb865256
highlight current enabled CC item when video first loaded. r=jaws

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c620fb865256

Comment 12

[Hani Yacoub, Desktop QA]

Build ID: 20170203030204
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

Verified as fixed on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64 on Firefox Nightly 54.0a1.

Comment 13

[Ray Lin[:ralin]]

Comment on attachment 8829337 [details]
Bug 1332994 - highlight current enabled CC item when video first loaded.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 887934
[User impact if declined]: enabled tracked won't be highlighted correctly
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: only correct existing track item's mode
[String changes made/needed]: none

Thanks

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8829337 [details]
Bug 1332994 - highlight current enabled CC item when video first loaded.

Fix a menu display issue in video. Aurora53+.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/098f633018d5

Comment 16

[Ray Lin[:ralin]]

Comment on attachment 8829337 [details]
Bug 1332994 - highlight current enabled CC item when video first loaded.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 887934
[User impact if declined]: enabled tracked won't be highlighted correctly
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: only correct existing track item's mode
[String changes made/needed]: none

Same request ask for uplift to beta, thanks :)

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8829337 [details]
Bug 1332994 - highlight current enabled CC item when video first loaded.

fix issue with closed captions in beta52

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Needs a rebased patch for Beta uplift.

Comment 19

[Ray Lin[:ralin]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Needs a rebased patch for Beta uplift.

Sorry, it's my first time uplift conflicted patch to beta. Should I just override the existing attachment with rebased(on beta) one? Thank you

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I believe you can submit a beta-only patch as another reviewboard entry. Ping #mozreview for help if otherwise...

Comment 21

[Ray Lin[:ralin]]

Attachment: Bug-1332994-highlight-current-enabled-CC-item-when-v.patch

Hi Jared,

Sorry for asking you review again. Mozreview seems to stop me from pushing new request if there's already a submitted one, so I create another patch for beta approval here. 

Thanks :-)

Comment 22

[Ray Lin[:ralin]]

Comment on attachment 8836752 [details] [diff] [review]
Bug-1332994-highlight-current-enabled-CC-item-when-v.patch


Approval Request Comment
[Feature/Bug causing the regression]: Bug 887934
[User impact if declined]: enabled tracked won't be highlighted correctly
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: only correct existing track item's mode
[String changes made/needed]: none


carry over the same approval-beta request for beta-only patch, thanks.

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8836752 [details] [diff] [review]
Bug-1332994-highlight-current-enabled-CC-item-when-v.patch

rebased patch, beta52+

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b79afbae1a16

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/b79afbae1a16

Comment 26

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Ray's assessment on manual testing needs (Comment 22) and the fact that this patch has automated coverage.