1391791 - Audio (controls) element blows up in width if a padding is set

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[didu]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170814072924

Steps to reproduce:

Create an audio tag with controls enabled, e.g. `<audio controls></audio>`, and give it some padding through CSS, e.g. `audio { padding: 1em; }`.

Example: https://jsfiddle.net/Ln77zyLp/

The problem seems new in 55 and occurs at least on the 64-bit Linux and the 32-bit Windows versions.


Actual results:

Upon page load, the controls element starts visibly growing in width. It finally stops when reaching a width of roughly 50000px (which is usually after a few seconds).


Expected results:

The audio controls element should:

- not change its size after initial rendering
- respect parent (block) element boundaries, in a block or inline fashion

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: audio control layout is broken and performance as well as high CPU usage due to regression

Regression window:
1st regression
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6ec07b1cf72135578fbfe26261be8c66cbe3874a&tochange=62f4ce5004d4e7874a27c41cc0b18cd31fbf9206

2nd regression(height only improved)
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c82858285f0b37cb3f6e16245d5e993a50102e57&tochange=e999e5ea1f78dd6f89ae485c199477083b369946



Regressed by: Bug 1373537, Bug 1382218


:ralin,
Your patch seems to be cause the problem. Could you look this?

Comment 2

[Ray Lin[:ralin]]

np, I guess we need the same way fixing height padding on width.

Comment 3

[Gerry Chang [:gchang]]

Track 56+/57+ as regression.

Comment 4

[Ray Lin[:ralin]]

NI myself, I'll work on it tomorrow.

Comment 5

[Ray Lin[:ralin]]

Attachment: Bug 1391791 - Calculate controlBar width without counting toward audio padding.

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

Comment 6

[Ray Lin[:ralin]]

Try result looks fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecd5bd086f122fad58728b3e51c615a89d1e4c31

Comment 7

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

Comment on attachment 8899868 [details]
Bug 1391791 - Calculate controlBar width without counting toward audio padding.

https://reviewboard.mozilla.org/r/171200/#review177038

Comment 8

[Ray Lin[:ralin]]

Thank you Jared :)

Comment 9

[Julien Cristau [:jcristau]]

How common do we think this is?  (Trying to figure out if it's worth considering for dot release inclusion, or if this is wontfix for 55)

Comment 10

[Ray Lin[:ralin]]

(In reply to Julien Cristau [:jcristau] from comment #9)
> How common do we think this is?  (Trying to figure out if it's worth
> considering for dot release inclusion, or if this is wontfix for 55)

I don't think ppl often set padding to audio directly, but the page UI would be totally broken once happened, so I would consider  dot release. Thanks.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50a42e71c69a
Calculate controlBar width without counting toward audio padding. r=jaws

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/50a42e71c69a

Comment 13

[Ray Lin[:ralin]]

Comment on attachment 8899868 [details]
Bug 1391791 - Calculate controlBar width without counting toward audio padding.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1373537
[User impact if declined]: please see comment 1: audio control layout is broken and performance as well as high CPU usage due to regression
[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?]: few lines code enhances the calculation of control bar width.
[String changes made/needed]: none


Thanks :)

Comment 14

[Sylvestre Ledru [:Sylvestre]]

This patch doesn't apply to m-b:    

grafting 419044:50a42e71c69a "Bug 1391791 - Calculate controlBar width without counting toward audio padding. r=jaws"
merging toolkit/content/tests/reftests/reftest.list
 warning: conflicts while merging toolkit/content/tests/reftests/reftest.list! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 15

[Ray Lin[:ralin]]

Comment on attachment 8899868 [details]
Bug 1391791 - Calculate controlBar width without counting toward audio padding.

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

Comment 16

[Ray Lin[:ralin]]

Thank you Sylvestre,

I've rebased the patch on top of m-b, could you help me to apply again?

Comment 17

[Gerry Chang [:gchang]]

Hi Alice,
Can you help check if this issue was fixed in the latest nightly?

Comment 18

[Alice0775 White]

I can confirm that the problem is fixed.
I can no longer reproduce the problem on the following Nightly build.
 
Build ID 	20170827100428
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 19

[Ray Lin[:ralin]]

Thanks for your help Gerry, Alice.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8899868 [details]
Bug 1391791 - Calculate controlBar width without counting toward audio padding.

Fix a regression and was verified. 
Alice, thanks. Beta56+.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0ae11b062cb4

Comment 23

[Andrei Vaida [:avaida]]

(In reply to Ray Lin[:ralin] from comment #13)
> [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's assessment on manual testing needs and the fact that this fix has automated coverage.