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

VERIFIED FIXED in Firefox 52

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: ralin, Assigned: ralin)

Tracking

53 Branch
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 verified)

Details

Attachments

(2 attachments)

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.
Blocks: 1333103
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.
Attachment #8829337 - Flags: review?(jaws) → review-
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 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, {});
Attachment #8829337 - Flags: review?(jaws) → review+
Keywords: checkin-needed
seems there is one open issue in mozreview that needs be to be fixed before we can use autoland. Can you fix this, thanks!
Flags: needinfo?(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 :)
Flags: needinfo?(ralin)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c620fb865256
highlight current enabled CC item when video first loaded. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c620fb865256
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1333103
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.
Status: RESOLVED → VERIFIED
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
Attachment #8829337 - Flags: approval-mozilla-aurora?
Comment on attachment 8829337 [details]
Bug 1332994 - highlight current enabled CC item when video first loaded.

Fix a menu display issue in video. Aurora53+.
Attachment #8829337 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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 :)
Attachment #8829337 - Flags: approval-mozilla-beta?
Comment on attachment 8829337 [details]
Bug 1332994 - highlight current enabled CC item when video first loaded.

fix issue with closed captions in beta52
Attachment #8829337 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs a rebased patch for Beta uplift.
Flags: needinfo?(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
Flags: needinfo?(ralin) → needinfo?(ryanvm)
I believe you can submit a beta-only patch as another reviewboard entry. Ping #mozreview for help if otherwise...
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 :-)
Flags: needinfo?(ryanvm)
Attachment #8836752 - Flags: review?(jaws)
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.
Attachment #8836752 - Flags: approval-mozilla-beta?
Comment on attachment 8836752 [details] [diff] [review]
Bug-1332994-highlight-current-enabled-CC-item-when-v.patch

rebased patch, beta52+
Attachment #8836752 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8829337 - Flags: approval-mozilla-beta+
Setting qe-verify- based on Ray's assessment on manual testing needs (Comment 22) and the fact that this patch has automated coverage.
You need to log in before you can comment on or make changes to this bug.