1339269 - Video/Audio controls don't layout correctly, if size is initially 0

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

(I'm spinning this off as a helper for bug 1319653, to start with a simpler testcase & a clean slate for discussion now that we know some of what the problem is.)

STR:
 1. Load testcase.

EXPECTED RESULTS:
 Controls should be visible.

ACTUAL RESULTS:
 Controls are not visible.


(I think this is basically due to the fact that videocontrols.xml reads controls.clientWidth early on, and then caches it, and then takes an early-return in adjustControlSize() if the cached value happens to be 0.  This means we never actually adjust the control size if the initial value happens to have been 0.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: reference case 1

Comment 2

[Daniel Holbert [:dholbert]]

(Note: if you open devtools after loading this bug's testcase, that seems to prompt us to re-do some XBL stuff which makes the testcase fix itself. So: if you do open devtools, don't let that fool you -- and you can still make the bug reproduce by just reloading after devtools are open, I think.)

Comment 3

[Daniel Holbert [:dholbert]]

> ACTUAL RESULTS:
> Controls are not visible.
...or they overflow and are awkwardly clipped.  (In older nightlies, soon after bug 1271765 landed, they overflow.  In more recent Nightlies, they're entirely missing.)

I verified that this regressed from bug 1271765, with some Nightly spot checks around the range when it landed. --> Marking as a regression from that bug.

[Tracking Requested - why for this release]: Regression in video/audio controls functionality/usability.

Comment 4

[Ray Lin[:ralin]]

Attachment: Bug 1339269 - Enhance controls adjustment against delay layout reflow.

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

Comment 5

[Daniel Holbert [:dholbert]]

jaws, if you have cycles to prioritize this review, that'd be great!  I'd love to avoid shipping this regression beyond Aurora, if we can.  It's an easy to trigger funtional regression, even without a silly 0-sized audio control, as demonstrated by ralin's testcases linked on bug 1319653.  (The initially-0-sized-control scenario in the testcase here is just one easy-to-reason-about version of this issue.)

(Currently this just affects Aurora + Nightly, but it'll affect Beta pretty soon, and uplifts will get progressively riskier as more time passes after that.)

Comment 6

[Ray Lin[:ralin]]

Comment on attachment 8838429 [details]
Bug 1339269 - Enhance controls adjustment against delay layout reflow.

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

Comment 7

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

Comment on attachment 8838429 [details]
Bug 1339269 - Enhance controls adjustment against delay layout reflow.

https://reviewboard.mozilla.org/r/113376/#review116464

rs=me

::: toolkit/content/widgets/videocontrols.xml:1151
(Diff revision 2)
> +        // For the controls which haven't got correct computed size yet, force
> +        // them re-cache their minWidth when video resized (reflow).

s/them re-cache/them to re-cache/

s/when video resized/when the media is resized/

Comment 8

[Ray Lin[:ralin]]

Comment on attachment 8838429 [details]
Bug 1339269 - Enhance controls adjustment against delay layout reflow.

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

Comment 9

[Ray Lin[:ralin]]

Thank you Daniel, Jared :)

----
The issue is fixed, latest try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=430a17a78f66de1358e794dabb613b0e989df178&selectedJob=79647202

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/baff63f434f7
Enhance controls adjustment against delay layout reflow. r=jaws

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 13

[Ray Lin[:ralin]]

Comment on attachment 8838429 [details]
Bug 1339269 - Enhance controls adjustment against delay layout reflow.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1271765
[User impact if declined]: External stylesheet or delay reflow would break video control layout.
[Is this code covered by automated tests?]: reftest mentioned in Bug 1319653.
[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?]: no extra features are added, only tweaked the controls adjustment. 
[String changes made/needed]: none

Thanks!

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8838429 [details]
Bug 1339269 - Enhance controls adjustment against delay layout reflow.

Polish a video/audio controls layout issue. Aurora53+.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a3fe876dacfb

Comment 16

[Andrei Vaida [:avaida]]

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