Closed
Bug 1332994
Opened 8 years ago
Closed 8 years ago
The subtitle menu should highlight current enabled CC when video first loaded
Categories
(Toolkit :: Video/Audio Controls, defect)
Tracking
()
VERIFIED
FIXED
mozilla54
People
(Reporter: ralin, Assigned: ralin)
References
Details
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
jaws
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
3.19 KB,
patch
|
jaws
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•8 years ago
|
status-firefox51:
--- → wontfix
status-firefox52:
--- → affected
Comment hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 4•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
(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
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c620fb865256
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 12•8 years ago
|
||
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
Assignee | ||
Comment 13•8 years ago
|
||
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 14•8 years ago
|
||
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 15•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/098f633018d5
Assignee | ||
Comment 16•8 years ago
|
||
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 17•8 years ago
|
||
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+
Assignee | ||
Comment 19•8 years ago
|
||
(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)
Comment 20•8 years ago
|
||
I believe you can submit a beta-only patch as another reviewboard entry. Ping #mozreview for help if otherwise...
Assignee | ||
Comment 21•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8836752 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 22•8 years ago
|
||
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 23•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8829337 -
Flags: approval-mozilla-beta+
Comment 24•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/b79afbae1a16
Comment 25•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/b79afbae1a16
status-firefox-esr52:
--- → fixed
Comment 26•8 years ago
|
||
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.
Description
•