Closed Bug 1339269 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 ? fixed
firefox54 --- fixed

People

(Reporter: dholbert, Assigned: ralin)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached file 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.)
Attached file reference case 1
(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.)
> 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.
Blocks: 1271765
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Status: NEW → ASSIGNED
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.)
Flags: needinfo?(jaws)
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/
Attachment #8838429 - Flags: review?(jaws) → review+
Flags: needinfo?(jaws)
Thank you Daniel, Jared :)

----
The issue is fixed, latest try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=430a17a78f66de1358e794dabb613b0e989df178&selectedJob=79647202
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/baff63f434f7
Enhance controls adjustment against delay layout reflow. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/baff63f434f7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(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!
Flags: needinfo?(ralin)
Attachment #8838429 - Flags: approval-mozilla-aurora?
Comment on attachment 8838429 [details]
Bug 1339269 - Enhance controls adjustment against delay layout reflow.

Polish a video/audio controls layout issue. Aurora53+.
Attachment #8838429 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Setting qe-verify- based on Ray's assessment on manual testing needs (see Comment 13) and the fact that this fix has automated coverage.
Flags: qe-verify-
Depends on: 1362146
No longer depends on: 1362146
You need to log in before you can comment on or make changes to this bug.