1382218 - Huge media controls

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[[:philipp]]

we got a report in the german community support forum that after the update to 55.0b10 the site at http://gosebru.ch/bilderbuch.html is exposing huge media controls pushing other content out of view.

running mozregression on this issue points to the uplifting of bug 1373537 to beta as source of the problem.

Comment 1

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

[Tracking Requested - why for this release]: potential to break webpage layouts with very large video controls (the linked testcase has a <video> element that is 47,536px tall)

Ray Lin, can you work on this? I can reproduce this on Nightly

Comment 2

[Ray Lin[:ralin]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> [Tracking Requested - why for this release]: potential to break webpage
> layouts with very large video controls (the linked testcase has a <video>
> element that is 47,536px tall)
> 
> Ray Lin, can you work on this? I can reproduce this on Nightly

No problem, I just got a patch for that. I'll do more test tomorrow. Thanks

Comment 3

[Julien Cristau [:jcristau]]

recent regression in beta, marking as blocker for 55, we should either fix this or backout the previous uplift (preferrably by monday so this can get fixed in 55.0b12).

Comment 4

[Ray Lin[:ralin]]

Attachment: Bug 1382218 - Cap the height of <audio> controlBar as well to get rid of endless reflow.

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

Comment 5

[Ray Lin[:ralin]]

It occurs that the padding-top triggers an endless self-invoked reflow and keep the height growing. In the patch, I make the controlBar height be capped with the bounds between 40px ~ 28px, and that should be able to make sure the adjustment won't be interfered by the rules that can affect size computation, such as padding or margin.

Jared, could you help me to review this patch? Thanks.

Comment 6

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

Comment on attachment 8888340 [details]
Bug 1382218 - Cap the height of <audio> controlBar as well to get rid of endless reflow.

https://reviewboard.mozilla.org/r/159282/#review164764

::: toolkit/content/widgets/videocontrols.xml:1685
(Diff revision 1)
> +            let controlBarHeight = givenHeight;
> +            if (givenHeight >= this.controlBarMinHeight) {
> +              controlBarHeight = this.controlBarMinHeight;
> +            } else if (givenHeight <= this.controlBarMinVisibleHeight) {
> +              controlBarHeight = this.controlBarMinVisibleHeight;
> +            }

Please replace this all with:

> let controlBarHeight = Math.max(Math.min(givenHeight, this.controlBarMinHeight), this.controlBarMinVisibleHeight);

Comment 7

[Ray Lin[:ralin]]

Comment on attachment 8888340 [details]
Bug 1382218 - Cap the height of <audio> controlBar as well to get rid of endless reflow.

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

Comment 8

[Ray Lin[:ralin]]

The issue is fixed, and looks cleaner now, thank you Jared.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e999e5ea1f78
Cap the height of <audio> controlBar as well to get rid of endless reflow. r=jaws

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Ray Lin[:ralin]]

Comment on attachment 8888340 [details]
Bug 1382218 - Cap the height of <audio> controlBar as well to get rid of endless reflow.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1373537
[User impact if declined]: As mentioned in bug, huge media controls presents.
[Is this code covered by automated tests?]: yes, the reftest introduced in patch 
[Has the fix been verified in Nightly?]: no, but verified by myself
[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?]: only add one more constraint to audio control's height.  
[String changes made/needed]: none

Thanks :D

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8888340 [details]
Bug 1382218 - Cap the height of <audio> controlBar as well to get rid of endless reflow.

media controls regression fix, beta55+

should be in 55.0b12

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/436dee8a429b

Comment 14

[Andrei Vaida [:avaida]]

(In reply to Ray Lin[:ralin] from comment #11)
> [Is this code covered by automated tests?]: yes, the reftest introduced in
> patch 
> [Has the fix been verified in Nightly?]: no, but verified by myself
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Ray Lin's assessment on manual testing needs and the fact that this fix has automated coverage.