Closed Bug 1347673 Opened 5 years ago Closed 5 years ago

"CC" button in video controls causes overflow (or is missing), when subtitles/text track is dynamically added

Categories

(Toolkit :: Video/Audio Controls, defect)

53 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: dholbert, Assigned: ralin)

References

(Blocks 1 open bug)

Details

(Keywords: regression, testcase)

Attachments

(3 files)

STR:
 0. DO NOT have devtools open. That seems to triggers extra layout which makes the problem go away.
 1. Load attached testcase.

EXPECTED RESULTS:
Both videos below should show a crossed-out "CC" button, and the controls should not overflow.

ACTUAL RESULTS:
The second video has broken controls:
- In Nightlies before bug 1339269 landed, the CC button just never shows up.
- In Nightlies after bug 1339269 landed, the CC button shows up BUT it pushes other controls to overflow, and it disappears if I trigger dynamic style changes e.g. by triggering a 'width' tweak via some hover styles (as I'll demonstrate in a later testcase).

This is a regression from bug 1271765, which changed how it manifested when bug 1339269 landed.
Flags: needinfo?(ralin)
Attachment #8847749 - Attachment description: testcase 1: second video "CC" is missing or triggers overflow → testcase 1: second video "CC" is missing in older nightlies, & triggers overflow in newer nightlies
(In reply to Daniel Holbert [:dholbert] from comment #0)
> ACTUAL RESULTS:
> [...] it disappears if I trigger dynamic
> style changes e.g. by triggering a 'width' tweak via some hover styles (as
> I'll demonstrate in a later testcase).

Here's a testcase to demonstrate this.

Note that in both testcases here, the only difference is whether the "CC" button is added before vs. after the controls get laid out for the first time.
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Flags: needinfo?(ralin)
(In reply to Daniel Holbert [:dholbert] from comment #0)

Thank you Daniel, you pointed out a case we've not noticed for a while :P

There are few lines of unreasonable codes[0] in `adjustControlSize()` hide CC button from showing up, which cause invalid adjustment though we do fire re-adjustment when new tracks added.

See the link below. In testcases, CC button was flagged as "unwanted" when video initialized, and never has chance to change back to "true". To correct this, we simply need set `isWanted` to true in the opposite conditions.

[0] https://dxr.mozilla.org/mozilla-central/rev/ff04d410e74b69acfab17ef7e73e7397602d5a68/toolkit/content/widgets/videocontrols.xml#1713-1721
(In reply to Ray Lin[:ralin] from comment #2)
> Created attachment 8848031 [details]
> Bug 1347673 - Adjust layout accordingly for later enabled controls.
> 
> Review commit: https://reviewboard.mozilla.org/r/120996/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/120996/

It's good to have a test for this as well, perhaps just crib from Daniel's testcase.
I'll update the patch tomorrow.
(In reply to Ray Lin[:ralin] from comment #4)
> It's good to have a test for this as well, perhaps just crib from Daniel's
> testcase.
> I'll update the patch tomorrow.

A reftest would probably be nice.  My testcase relies on "setTimeout", which we try to avoid in reftests except when it's absolutely needed.  You'll want to replace my setTimeout with MozReftestInvalidate (and "reftest-wait" on the <html> element to delay the reftest snapshot).  Sample usage here:
https://dxr.mozilla.org/mozilla-central/source/layout/reftests/css-gradients/large-gradient-3.html

MozReftestInvalidate is a special event that gets fired *only in our reftest harness* after the page has been fully painted.
(In reply to Daniel Holbert [:dholbert] from comment #5)

Daniel, thank you again for the helpful hint :D
---

Hi Jared, 

The reftest has been added, could you help to review this patch? Thanks.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5d5e258b6994d7164ad11df1467945c8c27e57c
Comment on attachment 8848031 [details]
Bug 1347673 - Visibility state of extra video control buttons should be recalculated each time the video controls are adjusted.

https://reviewboard.mozilla.org/r/120996/#review123500

Thanks for the quick fix here! Two drive-by nits:

::: commit-message-ff04d:1
(Diff revision 2)
> +Bug 1347673 - Adjust layout accordingly for later enabled controls. r=jaws

s/controls/video controls/

Otherwise it's unclear what piece of the browser is changing here.

(Also, "Adjust layout accordingly" could perhaps be replaced with a more specific description of what you're changing, too -- it seems like this patch is mostly tweaking some "isWanted" bools? I'm not really familiar with this code, though, so I dunno.)

::: toolkit/content/tests/reftests/videocontrols-dynamically-add-cc.html:20
(Diff revision 2)
> +      // "adjustControlSize()" first, and then the layout is ready to have
> +      // reftest.

s/ready to have reftest/ready for the reftest snapshot/
Comment on attachment 8848031 [details]
Bug 1347673 - Visibility state of extra video control buttons should be recalculated each time the video controls are adjusted.

https://reviewboard.mozilla.org/r/120996/#review123912

Thanks for the quick fix. Can you use the following as the commit message (note the multiple lines)? Let me know if you disagree with my understanding.

"Bug 1347673 - Visibility state of extra video control buttons should be recalculated each time the video controls are adjusted. r=dholbert,jaws

Previously, once a button on the controls was determined as 'unwanted', it could never become 'wanted'."
Attachment #8848031 - Flags: review?(jaws) → review+
Comment on attachment 8848031 [details]
Bug 1347673 - Visibility state of extra video control buttons should be recalculated each time the video controls are adjusted.

https://reviewboard.mozilla.org/r/120996/#review123912

(Probably just r=jaws, not r=dholbert,jaws. I just left some drive-by comments, but I didn't actually review the code changes.)
Thank you Jared, Daniel, I'll update the patch based on your review/comments.


BTW, I found several e10s reftest failed on try result, but the differences looks unrelated to control bar. Perhaps, it's a bug?

link: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://archive.mozilla.org/pub/firefox/try-builds/ralin@mozilla.com-a5d5e258b6994d7164ad11df1467945c8c27e57c/try-macosx64-debug/try_yosemite_r7-debug_test-reftest-e10s-2-bm134-tests1-macosx-build574.txt.gz&only_show_unexpected=1 

I've pushed another patch to try server that covers irrelevant area with green mask (that's how other videocontrols reftests do), hope it will fix the problem.
(In reply to Ray Lin[:ralin] from comment #11)
> Thank you Jared, Daniel, I'll update the patch based on your review/comments.
> 
> 
> BTW, I found several e10s reftest failed on try result, but the differences
> looks unrelated to control bar. Perhaps, it's a bug?

It seems to affect all reftest runs (at first glance), not just e10s ones. Here's that Try run with the issue, for reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5d5e258b6994d7164ad11df1467945c8c27e57c

It's a mismatch in the fringe around the play/pause button on the center of the video area.  The failure seems to happen quite reliably, though it's visually imperceptible (max difference 1).

It's probably worth filing a bug and adding a reference to that bug in all tests that use this #mask hack right now, to make it clearer while we're doing that.

Also, for future reference -- for mismatches as subtle as this (extremely small "max difference"), it's better to just use a "fuzzy" annotation to allow for a maximum amount of mismatch, rather than to bulk up the text with a mask element.

So instead of...
> == videocontrols-dynamically-add-cc.html videocontrols-dynamically-add-cc-ref.html
...you'd want:
> # fuzzy because of bug NNN
> fuzzy(1,47) == videocontrols-dynamically-add-cc.html videocontrols-dynamically-add-cc-ref.html

If the other videocontrols reftests' masked-mismatches (the ones you mentioned) are all similarly-subtle, we should probably rip the masks out from all of them and add "fuzzy" annotations.  That would be worth doing in a test-only followup bug -- but for now this patch is fine as-is -- I don't want to gate the fix on that test-tweak given that the test is following an existing pattern that we can just fix all at once later on.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c52627150cc5
Visibility state of extra video control buttons should be recalculated each time the video controls are adjusted. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c52627150cc5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1350916
See Also: → 1350927
(In reply to Daniel Holbert [:dholbert] from comment #14)

Sorry for the late reply. 

It seems that the others reftests I mentioned in comment 11 are actually written in mochitests[0] using canvas.compare() to do something similar to reftest. 
So perhaps, before finding out the reason why the imperceptible differences occur in reftest, I'd like to:
1. convert these mochitests to regular reftests (bug 1350927)
2. remove mask hask and leverage fuzzy annotation. (bug 1350916)


> It's probably worth filing a bug and adding a reference to that bug in all tests that use this #mask hack right now, to make it clearer while we're doing that.
as long as these 2 bugs are landed, then we can add these bugs as references to a new bug that addresses the problem.


[0] https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/toolkit/content/tests/widgets/videocontrols_direction_test.js#52-84
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(ralin)
Version: unspecified → 53 Branch
(In reply to Ray Lin[:ralin] from comment #17)
Those mochitests might not even need fuzzy annotations (and perhaps can get rid of the mask), since their mask is there to cover up a throbber which I think no longer exists.  Anyway, we can sort this out in bug 1347673 I suppose.
(In reply to Daniel Holbert [:dholbert] from comment #19)
> Anyway, we can sort this out in bug 1347673 I suppose.
Sorry, mispaste -- I meant bug 1350927 / bug 1350916
Comment on attachment 8848031 [details]
Bug 1347673 - Visibility state of extra video control buttons should be recalculated each time the video controls are adjusted.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1271765
[User impact if declined]: user can not see CC btn if the text track is dynamically added.
[Is this code covered by automated tests?]: yes, reftest in this patch
[Has the fix been verified in Nightly?]: no
[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
[Why is the change risky/not risky?]: update visibility state only, won't harm the original adjustment feature
[String changes made/needed]: none

Thanks :D
Flags: needinfo?(ralin)
Attachment #8848031 - Flags: approval-mozilla-beta?
Attachment #8848031 - Flags: approval-mozilla-aurora?
Comment on attachment 8848031 [details]
Bug 1347673 - Visibility state of extra video control buttons should be recalculated each time the video controls are adjusted.

Fix a regression related to video control. Aurora54+ & Beta53+.
Attachment #8848031 - Flags: approval-mozilla-beta?
Attachment #8848031 - Flags: approval-mozilla-beta+
Attachment #8848031 - Flags: approval-mozilla-aurora?
Attachment #8848031 - Flags: approval-mozilla-aurora+
Setting qe-verify- based on Ray's assessment on manual testing needs (see Comment 21) and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.