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

Status: RESOLVED DUPLICATE of bug 962560

(Toolkit :: Video/Audio Controls, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 1

[Justin Dolske [:Dolske]]

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

[Justin Dolske [:Dolske]]

Attachment: 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.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Bah. I just noted that the isAudioOnly setter can affect style too. :/

Comment 4

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

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.

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Xidorn - did you forget to land this?

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

I suppose this question is for Dolske.

Comment 7

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

Comment on attachment 8720025 [details] [diff] [review]
Patch v.2

Let's just land this now. We should get more tests written though.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 9

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

This actually looks like it was fixed by bug 962560.