Closed
Bug 1391791
Opened 7 years ago
Closed 7 years ago
Audio (controls) element blows up in width if a padding is set
Categories
(Toolkit :: Video/Audio Controls, defect)
Tracking
()
VERIFIED
FIXED
mozilla57
People
(Reporter: didu, Assigned: ralin)
References
Details
(Keywords: perf, regression)
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
jaws
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
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•7 years ago
|
||
regression-window |
[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?
Status: UNCONFIRMED → NEW
status-firefox55:
--- → affected
status-firefox56:
--- → affected
status-firefox57:
--- → affected
status-firefox-esr52:
--- → unaffected
tracking-firefox55:
--- → ?
tracking-firefox56:
--- → ?
tracking-firefox57:
--- → ?
Component: Untriaged → Video/Audio Controls
Ever confirmed: true
Flags: needinfo?(ralin)
Keywords: perf,
regression
Product: Firefox → Toolkit
Assignee | ||
Comment 2•7 years ago
|
||
np, I guess we need the same way fixing height padding on width.
Assignee: nobody → ralin
Flags: needinfo?(ralin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
Try result looks fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecd5bd086f122fad58728b3e51c615a89d1e4c31
Flags: needinfo?(ralin)
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8899868 [details] Bug 1391791 - Calculate controlBar width without counting toward audio padding. https://reviewboard.mozilla.org/r/171200/#review177038
Attachment #8899868 -
Flags: review?(jaws) → review+
Comment 9•7 years ago
|
||
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)
Flags: needinfo?(ralin)
Assignee | ||
Comment 10•7 years ago
|
||
(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.
Flags: needinfo?(ralin)
Comment 11•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/50a42e71c69a Calculate controlBar width without counting toward audio padding. r=jaws
Keywords: checkin-needed
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/50a42e71c69a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 13•7 years ago
|
||
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 :)
Attachment #8899868 -
Flags: approval-mozilla-beta?
Comment 14•7 years ago
|
||
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')
Flags: needinfo?(ralin)
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
Thank you Sylvestre, I've rebased the patch on top of m-b, could you help me to apply again?
Flags: needinfo?(ralin)
Comment 17•7 years ago
|
||
Hi Alice, Can you help check if this issue was fixed in the latest nightly?
Flags: needinfo?(alice0775)
Comment 18•7 years ago
|
||
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
Flags: needinfo?(alice0775)
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 19•7 years ago
|
||
Thanks for your help Gerry, Alice.
Comment 20•7 years ago
|
||
Comment on attachment 8899868 [details] Bug 1391791 - Calculate controlBar width without counting toward audio padding. Fix a regression and was verified. Alice, thanks. Beta56+.
Attachment #8899868 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 22•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/0ae11b062cb4
Flags: in-testsuite+
Comment 23•7 years ago
|
||
(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.
Flags: qe-verify-
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•