Volume indicator on <audio> is reset to 100% when its display value changes




Video/Audio Controls
2 years ago
2 years ago


(Reporter: xidorn, Assigned: Dolske)



Firefox Tracking Flags

(firefox47 affected)



(1 attachment, 1 obsolete attachment)



2 years ago
Steps to reproduce:
1. open "data:text/html,<audio controls id='a'>"
2. change the volume to a different value, e.g. 50%
3. open Web Console, input "a.volume" to confirm the current value is not 1.0
4. enter "a.style.display = 'block'"

Expected result:
The volume indicator still shows the value we changed to.

Actual result:
The volume indicator is reseted to 100%, while a.volume still returns the original value
Component: Audio/Video → Video/Audio Controls
Product: Core → Toolkit

Comment 1

2 years ago
Hmm, the binding does get reattached when .display is changed (as expected), but the setupInitialState() code is setting the right volume level.

Oh, I see. Bug 649490 changed how we display the volume. The |volumechanged| event listener handles updates correctly, but setupInitialState() doesn't. It needs the volumeForeground.style.paddingRight code. And, hmm, _volumeControlWidth hasn't been computed yet, so a bit more as well.

But that seems to be the core issue.

Comment 2

2 years ago
Created attachment 8720021 [details] [diff] [review]
Patch v.1

I realized, when looking at the block of default-size fetches, that we have a latent bug with _durationLabelWidth -- we never update the value, and it was already being too early for the media to have loaded a correct value. So it's always just computing the width of "0:00". So if the duration becomes a large value (showing hours), adjustControlSize() probably won't do right thing.

I think all the other default-size computations are ok, because they're static.
Assignee: nobody → dolske
Attachment #8720021 - Flags: review?(jaws)

Comment 3

2 years ago
Created attachment 8720025 [details] [diff] [review]
Patch v.2

Bah. I just noted that the isAudioOnly setter can affect style too. :/
Attachment #8720021 - Attachment is obsolete: true
Attachment #8720021 - Flags: review?(jaws)
Attachment #8720025 - Flags: review?(jaws)
Comment on attachment 8720025 [details] [diff] [review]
Patch v.2

Review of attachment 8720025 [details] [diff] [review]:

Please use mozreview to request reviews going forward, as it allows me to get more context on the file when looking at how code has moved from one section to another.

The patch looks fine but we should include a test that covers this.
Attachment #8720025 - Flags: review?(jaws) → feedback+
Xidorn - did you forget to land this?
Flags: needinfo?(quanxunzhen)

Comment 6

2 years ago
I suppose this question is for Dolske.
Flags: needinfo?(quanxunzhen) → needinfo?(dolske)
Comment on attachment 8720025 [details] [diff] [review]
Patch v.2

Let's just land this now. We should get more tests written though.
Flags: needinfo?(dolske)
Attachment #8720025 - Flags: feedback+ → review+
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
This actually looks like it was fixed by bug 962560.
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 962560
You need to log in before you can comment on or make changes to this bug.