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

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: enverest, Assigned: ralin)

Tracking

({regression})

56 Branch
mozilla58
regression
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56+ wontfix, firefox57+ fixed, firefox58 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
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.
Component: Untriaged → Layout
Keywords: regression, regressionwindow-wanted
Product: Firefox → Core

Comment 1

2 years ago
[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.
Blocks: 1391791
Status: UNCONFIRMED → NEW
status-firefox55: --- → unaffected
status-firefox56: --- → affected
status-firefox57: --- → affected
status-firefox-esr52: --- → unaffected
tracking-firefox56: --- → ?
Component: Layout → Video/Audio Controls
Ever confirmed: true
Flags: needinfo?(ralin)
Keywords: regressionwindow-wanted
Product: Core → Toolkit

Comment 2

2 years ago
Posted file testcase html

Comment 3

2 years ago
Posted image screenshot
Track 56+ as regression.
tracking-firefox56: ? → +
(Assignee)

Comment 5

2 years ago
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 :)
Assignee: nobody → ralin
Flags: needinfo?(ralin)
status-firefox56: affected → wontfix
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
See Also: → bug 1403093
[Tracking Requested - why for this release]: layout regression, didn't get fixed in time for 56
tracking-firefox57: --- → ?
let's see if we can get this fixed in time for 57.
status-firefox58: --- → affected
tracking-firefox57: ? → +
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
mochitest added. Jared, could you help me review the patch? Thanks.
Comment hidden (mozreview-request)
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.
Attachment #8909256 - Flags: review?(jaws) → review+
(Assignee)

Comment 14

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Comment 16

2 years ago
thanks
Keywords: checkin-needed

Comment 17

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1623a74b17bd
Update controlBar width according to videocontrols which really grows along <audio> width. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1623a74b17bd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Hi Ray, should we uplift this fix to Beta57?
Flags: needinfo?(ralin)
(Assignee)

Comment 20

2 years ago
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.
Flags: needinfo?(ralin)
Attachment #8909256 - Flags: approval-mozilla-beta?
(Assignee)

Comment 21

2 years ago
Hey Alice, 

Could you help me to verify the issue is fixed in your test case? Thanks.
Flags: needinfo?(alice0775)
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
Attachment #8909256 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 23

2 years ago
(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
Flags: needinfo?(alice0775)

Comment 24

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/6966674d5278
status-firefox57: affected → fixed
Flags: in-testsuite+
(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.
Flags: qe-verify-
Duplicate of this bug: 1406719
Duplicate of this bug: 1408802
Duplicate of this bug: 1409552
Duplicate of this bug: 1411940
You need to log in before you can comment on or make changes to this bug.