1397486 - audio tag doesn't listen to CSS width [ff 56]

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[enverest]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170903140023

Steps to reproduce:

1) Got to website http://scalalaz.ru/series-28.html
2) Look to a width of audio on the page.
3) Look to its width in DevTools.
4) Try to change width.


Actual results:

Width is too small. And not changing size.


Expected results:

It has bigger (normal) size in Firefox 55, Firefox 52 ESR and Chrome browsers (Ubuntu and MacOS).
Changing width in CSS changes size of the audio player in other browsers.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: layout regression

Regression window
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=39b92731bbdf977a0dbd66592d421ab99de777e2&tochange=50a42e71c69a2cf98bc760a849103836c8edf901

Regressed by;50a42e71c69a	Ray Lin — Bug 1391791 - Calculate controlBar width without counting toward audio padding. r=jaws


:ralin,

Your patch seems cause the problem. Could you look this.

Comment 2

[Alice0775 White]

Attachment: testcase html

Comment 3

[Alice0775 White]

Attachment: screenshot

Comment 4

[Gerry Chang [:gchang]]

Track 56+ as regression.

Comment 5

[Ray Lin[:ralin]]

Taking this bug, also I should have added a test for width in <audio> like what we've done for <video>, in case any fix on size adjustment regress it in the future...  Thank you enverest, Alice :)

Comment 6

[Ray Lin[:ralin]]

Attachment: Bug 1397486 - Update controlBar width according to videocontrols which really grows along <audio> width.

This patch can fix the issue, but still lack a test to prevent
regression in the future.

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

Comment 8

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

[Tracking Requested - why for this release]: layout regression, didn't get fixed in time for 56

Comment 9

[Julien Cristau [:jcristau]]

let's see if we can get this fixed in time for 57.

Comment 10

[Ray Lin[:ralin]]

Comment on attachment 8909256 [details]
Bug 1397486 - Update controlBar width according to videocontrols which really grows along <audio> width.

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

Comment 11

[Ray Lin[:ralin]]

mochitest added. Jared, could you help me review the patch? Thanks.

Comment 12

[Ray Lin[:ralin]]

Comment on attachment 8909256 [details]
Bug 1397486 - Update controlBar width according to videocontrols which really grows along <audio> width.

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

Comment 13

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

Comment on attachment 8909256 [details]
Bug 1397486 - Update controlBar width according to videocontrols which really grows along <audio> width.

https://reviewboard.mozilla.org/r/180852/#review188826

::: toolkit/content/tests/widgets/head.js:30
(Diff revision 2)
> -  const videoControl = domUtils.getChildrenForNode(video, true)[1];
> +  // <videocontrols> is the second anonymous child of <video>, but
> +  // the first child of <audio>.
> +  const videoControlIndex = video.nodeName == "VIDEO" ? 1 : 0;

Can you point me to where this is? http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/layout/style/res/html.css#740 doesn't seem any different for <video> vs <audio>.

What's the first anonymous child of <video> ?

::: toolkit/content/tests/widgets/test_audiocontrols_dimensions.html:37
(Diff revision 2)
> -  var audio = document.getElementById("audio");
> +  add_task(async function check_audio_height() {
> +    is(audio.clientHeight, 40, "checking height of audio element");
> +  });
>  
> -  SpecialPowers.pushPrefEnv({"set": [["media.cache_size", 40000]]}, startTest);
> -  function startTest() {
> +  add_task(async function check_controlbar_width() {
> +    const originalControlBarWidth = controlBar.clientWidth;

Can you add a test here to confirm that the width is not 400? Otherwise the latter test cases would confusingly fail in the future if the default width became 400px.

::: toolkit/content/widgets/videocontrols.xml:1718
(Diff revision 2)
> +          // This can help us expand the control and restore to default size in the next time adjustment when
> +          // undo the size change.

The wording here is a bit confusing.

"in the next time adjustment when undo the size change"

Perhaps you could say:
This can help us expand the control and restore to the default size the next time we need to adjust the sizing.

Comment 14

[Ray Lin[:ralin]]

Comment on attachment 8909256 [details]
Bug 1397486 - Update controlBar width according to videocontrols which really grows along <audio> width.

https://reviewboard.mozilla.org/r/180852/#review188826

Thank you for the review :D

> Can you point me to where this is? http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/layout/style/res/html.css#740 doesn't seem any different for <video> vs <audio>.
> 
> What's the first anonymous child of <video> ?

I should use "firt anonymous node" instead.

Here shows three anonymous nodes in videoFrame:
http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/layout/generic/nsVideoFrame.cpp#159-174
, poster is only available for <video>, which makes the difference.

> Can you add a test here to confirm that the width is not 400? Otherwise the latter test cases would confusingly fail in the future if the default width became 400px.

no problem, will add

> The wording here is a bit confusing.
> 
> "in the next time adjustment when undo the size change"
> 
> Perhaps you could say:
> This can help us expand the control and restore to the default size the next time we need to adjust the sizing.

looks much better, thanks

Comment 15

[Ray Lin[:ralin]]

Comment on attachment 8909256 [details]
Bug 1397486 - Update controlBar width according to videocontrols which really grows along <audio> width.

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

Comment 16

[Ray Lin[:ralin]]

thanks

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1623a74b17bd
Update controlBar width according to videocontrols which really grows along <audio> width. r=jaws

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1623a74b17bd

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Ray, should we uplift this fix to Beta57?

Comment 20

[Ray Lin[:ralin]]

Comment on attachment 8909256 [details]
Bug 1397486 - Update controlBar width according to videocontrols which really grows along <audio> width.

Thanks for reminding that, Ritu

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1391791
[User impact if declined]: media controls doesn't visually change the size after setting width 
[Is this code covered by automated tests?]: yes
[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?]: so far the layout adjusting test cases are covered by automated test,  unlikely this change would regress something serious.
[String changes made/needed]: none

Thanks.

Comment 21

[Ray Lin[:ralin]]

Hey Alice, 

Could you help me to verify the issue is fixed in your test case? Thanks.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8909256 [details]
Bug 1397486 - Update controlBar width according to videocontrols which really grows along <audio> width.

Recent regression, taking it.
Should be in 57b5

Comment 23

[Alice0775 White]

(In reply to Ray Lin[:ralin] from comment #21)
> Hey Alice, 
> 
> Could you help me to verify the issue is fixed in your test case? Thanks.


I can reproduce the problem on 
Build ID 	20170825002024
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0


And I can verify that the problem is fixed on the following Nightly58.0a1.
Build ID 	20171001220301
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6966674d5278

Comment 25

[Andrei Vaida [:avaida]]

(In reply to Ray Lin[:ralin] from comment #20)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: no
> [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.