Closed
Bug 1397486
Opened 7 years ago
Closed 7 years ago
audio tag doesn't listen to CSS width [ff 56]
Categories
(Toolkit :: Video/Audio Controls, defect)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | + | wontfix |
firefox57 | + | fixed |
firefox58 | --- | fixed |
People
(Reporter: enverest, Assigned: ralin)
References
Details
(Keywords: regression)
Attachments
(3 files)
969 bytes,
text/html
|
Details | |
56.40 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
jaws
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
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.
Updated•7 years ago
|
![]() |
||
Comment 1•7 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•7 years ago
|
||
![]() |
||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 5•7 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)
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
[Tracking Requested - why for this release]: layout regression, didn't get fixed in time for 56
tracking-firefox57:
--- → ?
Comment 9•7 years ago
|
||
let's see if we can get this fixed in time for 57.
status-firefox58:
--- → affected
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
mochitest added. Jared, could you help me review the patch? Thanks.
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
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•7 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) |
Comment 17•7 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
![]() |
||
Comment 18•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Hi Ray, should we uplift this fix to Beta57?
Flags: needinfo?(ralin)
Assignee | ||
Comment 20•7 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•7 years ago
|
||
Hey Alice,
Could you help me to verify the issue is fixed in your test case? Thanks.
Flags: needinfo?(alice0775)
Comment 22•7 years ago
|
||
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•7 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•7 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
Comment 25•7 years ago
|
||
(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-
You need to log in
before you can comment on or make changes to this bug.
Description
•