689374 - Specialized HTML5 audio controls for small-dimensions

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

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

Similar to bug 462117, we need to have a way to either reduce the amount of visible controls or restyle them to fit smaller dimensions when HTML5 <audio> tags have widths & heights that don't fit the default sizes.

Comment 1

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

This bug is similar to bug 462117, except we can't base our calculations off of |videoHeight| and |videoWidth|. We should be able to use much of the same code, but will just have to depend on different width/height for our calculations.

Comment 2

[Panu Horsmalahti]

I've created a test page by modifying a test page from the earlier bug ( http://panuhorsmalahti.fi/audiodimensions.html ). I'll take a shot at creating a patch later.

Comment 3

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

(In reply to Panu Horsmalahti from comment #2)
> I've created a test page by modifying a test page from the earlier bug (
> http://panuhorsmalahti.fi/audiodimensions.html ). I'll take a shot at
> creating a patch later.

Cool, please let me know if there is anything I can do to help you out with getting started.

Comment 4

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

The changes necessary to fix this bug should be made in /toolkit/content/widgets/videocontrols.xml

Comment 5

[Anurag Bhatnagar]

If nobody is currently working on this, I can pick it up.

Comment 6

[Josh Matthews [:jdm]]

Go for it!

Comment 7

[Bao Duong]

For a project in our Software Engineering class, we would like to work on this bug. Thank you!

Comment 8

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

Great! Let me know of any questions you may have :)

Comment 9

[Sean Chen]

Hey Jared, I dont know if Bao mentioned to you that I'm his partner for the Software Engineering class. I will take a look at this bug and let you know if I have any questions.

Comment 10

[Sean Chen]

Jared, So for the bug, the main issue is adjusting the visible controls to fit within the <audio> tags right? If so, the changes im proposing is to write a javascript function that can get the height and width and compare it to the default one and scale the controls accordingly with the height and width.

Comment 11

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

What do you mean by "the default one"? Basically, the buttons in the audio controls have a fixed width. The scrubber is the only flexible width item in the controls and that grows to consume the available space.

You can look at the code that was changed for bug 462117, https://hg.mozilla.org/mozilla-central/rev/5a45436b3c18, for an example.

Comment 12

[Sean Chen]

what i meant by the default one is the width and height for the <audio> tags. Thank you for the reference to bug 462117. which url link should I look at to see where the bug happens?

Comment 13

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

You should be able to use http://panuhorsmalahti.fi/audiodimensions.html for testing.

Comment 14

[Sean Chen]

ok thank you for the link. I'll start working on it soon and make the changes accordingly. Thanks.

Comment 15

[Sean Chen]

Hey Jared, i compared the submitted code from bug 462117 and see the changes inside videocontrols.css and videocontrols.xml. I was wondering what parts of the code I should focus on so that the visible controls fit the dimensions?

Comment 16

[Mardeg]

Tests should include how text from the <track> element appears from bug 833385.

Comment 17

[Sean Chen]

Attachment: First attempt at fixing the audio controls

Comment 18

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

Comment on attachment 754684 [details] [diff] [review]
First attempt at fixing the audio controls

This attachment doesn't show what has been added, removed, or changed.

You'll have to resubmit the patch following the steps that are described here: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 19

[Sean Chen]

Attachment: First attempt at fixing the audio controls

Comment 20

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

Comment on attachment 754890 [details] [diff] [review]
First attempt at fixing the audio controls

Review of attachment 754890 [details] [diff] [review]:
-----------------------------------------------------------------

This patch includes the code changes from bug 730626. Please separate these changes. This patch also changed a bunch of lines from having leading space characters to leading tab characters.

Please fix up these two issues and reupload.

Comment 21

[Sean Chen]

Attachment: First attempt at fixing the audio controls

Comment 22

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

Comment on attachment 755188 [details] [diff] [review]
First attempt at fixing the audio controls

Review of attachment 755188 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is based off a non-existent revision. Can you try getting a new clone of the mozilla-central tree and making your changes again?

Comment 23

[Sean Chen]

Attachment: First attempt at fixing the audio controls

Comment 24

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

Comment on attachment 755554 [details] [diff] [review]
First attempt at fixing the audio controls

Review of attachment 755554 [details] [diff] [review]:
-----------------------------------------------------------------

Much of this code is duplicated in the two cases. It seems that the only part that needs to be handled separately is calculating the height and width of the media element.

::: toolkit/content/widgets/videocontrols.xml
@@ +1337,5 @@
>                  _overlayPlayButtonWidth : 64,
>                  adjustControlSize : function adjustControlSize() {
> +                    if (this.isAudioOnly){
> +                        let audioHeight = this.video.clientHeight;
> +                        let audioWidth = this.video.clientWidth;

What values do audioHeight and audioWidth have?

If this patch works, then we can probably change nothing except removing the "if (this.isAudioOnly) return" code and renaming videoHeight and videoWidth to mediaHeight and mediaWidth, respectively.

Comment 25

[Sean Chen]

Attachment: Changed to mediaWidth and mediaHeight

Comment 26

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

What values do mediaWidth and mediaHeight have for <audio> elements? What happens when the page doesn't specify a size for the element?

Comment 27

[Sean Chen]

Attachment: Added console.log for debugging.

Comment 28

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

Comment on attachment 757011 [details] [diff] [review]
Added console.log for debugging.

Review of attachment 757011 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Sean, we don't need to check in these log statements. Do you have answers to the previous questions?

Comment 29

[Sean Chen]

I cannot see the values for mediaWidth and mediaHeight through console.log. Where would I be able to see the results from console.log? For the second question, are you talking about mediaWidth and mediaHeight when they don't specify a size?

Comment 30

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

(In reply to Sean Chen from comment #29)
> I cannot see the values for mediaWidth and mediaHeight through console.log.
> Where would I be able to see the results from console.log?

We talked about this over IRC.

> For the second
> question, are you talking about mediaWidth and mediaHeight when they don't
> specify a size?

Yes, when a webpage is simply:
<html><body><audio src="foo.mp4" controls/></body></html>

Comment 31

[Sean Chen]

Here are the results I have for mediaWidth and mediaHeight for the audio elements.

Media Height: 0 Media Width: 25
Media Height: 0 Media Width: 48
Media Height: 0 Media Width: 96
Media Height: 0 Media Width: 192
Media Height: 0 Media Width: 383
Media Height: 0 Media Width: 96
Media Height: 0 Media Width: 48
Media Height: 0 Media Width: 25

when the page doesn't specify a size for the <audio> element, it would use only the css code that is set up for the width and height percentages. 

The problem with the code now is that when I take out if(this.isAudioOnly) return; it would not show the audio controls and scrubber. What I wanted to do was to have it set up the width and height for the audio controls respectively and determine whether or not to reduce the size of the dimensions. I was wondering if I can get some feedback on how I can solve this problem.

Comment 32

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

Attachment: Patch

Bao or Sean, can you test out this patch? It seems to fix all the issues with the audio control sizing, as well as fixing issues with entering fullscreen and toggling playback for audio elements.

Comment 33

[Sean Chen]

Attachment: Patch v1.0

Comment 34

[Sean Chen]

Attachment: Patch v1.0

Comment 35

[Sean Chen]

Attachment: Patch v1.0

Comment 36

[Sean Chen]

Attachment: Patch v1.0

Comment 37

[Sean Chen]

Attachment: Patch v1.0

Comment 38

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

Comment on attachment 758430 [details] [diff] [review]
Patch v1.0

Review of attachment 758430 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/videocontrols.xml
@@ +1346,5 @@
>                  _playButtonWidth : 0,
>                  _durationLabelWidth : 0,
>                  _muteButtonWidth : 0,
>                  _fullscreenButtonWidth : 0,
> +                _controlBarHeight : 28,

I don't see why setting this to 28 will change anything. The initial value is overwritten in setupInitialState().

Comment 39

[Sean Chen]

Attachment: Patch v1.0

Comment 40

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

Comment on attachment 758718 [details] [diff] [review]
Patch v1.0

Review of attachment 758718 [details] [diff] [review]:
-----------------------------------------------------------------

What is the difference between this patch and the one that I uploaded?

Comment 41

[Sean Chen]

Apparently since you didnt think that _controlBarHeight set to 28 wont make a difference. I changed it back to 0. I tried out your patch and it worked fine once i set the height to 28px on the test file. So I was wondering what else should I change?

Comment 42

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

Well, we'll only need to change something if it's not working correctly. Unfortunately, we don't have a plan for what to do with media elements that have a height of 0, since we don't want to change the way the page is laid out.

I'll move my original patch forward for review.

Comment 43

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

Attachment: Patch v1.1

This patch combines a few different fixes for our audio controls. I can split them to separate bugs if you'd like, or we can just update the bug summary to reflect the various fixes.

Comment 44

[Justin Dolske [:Dolske]]

Comment on attachment 759349 [details] [diff] [review]
Patch v1.1

Review of attachment 759349 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/videocontrols.css
@@ +91,5 @@
>  
>  .controlBar[size="hidden"],
>  .controlBar[size="small"] .durationBox,
>  .controlBar[size="small"] .durationLabel,
> +.controlBar[size="small"] .scrubberStack,

Why is this changing from visibility:hidden to visibility:collapse?

::: toolkit/content/widgets/videocontrols.xml
@@ +1035,5 @@
>                  },
>  
>                  toggleFullscreen : function () {
> +                    if (this.isAudioOnly)
> +                        return;

This seems wrong. If it's an audio file, we just shouldn't show the FS button/menu instead of making it do nothing. And we have code to do that -- is it broken?

@@ +1068,5 @@
>  
>                  clickToPlayClickHandler : function(e) {
> +                    // Don't toggle playback for non-left clicks or for
> +                    // audio-only media since there is nothing visual to click on.
> +                    if (e.button != 0 || this.isAudioOnly)

Similar comment -- this handler is only attached to two elements...

- clickToPlay, which should be hidden for audio-only elements
- controlsSpacer, which should be height=0 for <audio>

I think only controlsSpacer would actually be clickable in the first place, for <video src=audio-only.ogg> since video gets a default height. Is there a reason to make clicking not work in this specific case?

::: toolkit/themes/osx/global/media/videocontrols.css
@@ +48,5 @@
>  }
>  
> +.controlBar[size="small"] > .playButton {
> +  margin-right: 0;
> +}

Hmm, was this broken for small-dimension <video>? *tests* oh dear. Did we goof this from the beginning, or did something else make this regress?

It's resetting the negative-margin used for the scrubber overlap, which has basically always been there... :|

Comment 45

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

(In reply to Justin Dolske [:Dolske] from comment #44)
> Comment on attachment 759349 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 759349 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/widgets/videocontrols.css
> @@ +91,5 @@
> >  
> >  .controlBar[size="hidden"],
> >  .controlBar[size="small"] .durationBox,
> >  .controlBar[size="small"] .durationLabel,
> > +.controlBar[size="small"] .scrubberStack,
> 
> Why is this changing from visibility:hidden to visibility:collapse?

Because <video> or <audio> that were 64px wide still couldn't fit the controls. Video doesn't matter much, who does that anyway lol?, but for <audio> I can picture people wanting a small 64px wide x 35px high element.

> ::: toolkit/content/widgets/videocontrols.xml
> @@ +1035,5 @@
> >                  },
> >  
> >                  toggleFullscreen : function () {
> > +                    if (this.isAudioOnly)
> > +                        return;
> 
> This seems wrong. If it's an audio file, we just shouldn't show the FS
> button/menu instead of making it do nothing. And we have code to do that --
> is it broken?

We now have dblclick on the controlsSpacer to enter fullscreen.

> @@ +1068,5 @@
> >  
> >                  clickToPlayClickHandler : function(e) {
> > +                    // Don't toggle playback for non-left clicks or for
> > +                    // audio-only media since there is nothing visual to click on.
> > +                    if (e.button != 0 || this.isAudioOnly)
> 
> Similar comment -- this handler is only attached to two elements...
> 
> - clickToPlay, which should be hidden for audio-only elements
> - controlsSpacer, which should be height=0 for <audio>
> 
> I think only controlsSpacer would actually be clickable in the first place,
> for <video src=audio-only.ogg> since video gets a default height. Is there a
> reason to make clicking not work in this specific case?

An audio element with a specified height that is greater than 28px will have a non-zero height controlsSpacer. That blank area shouldn't react to clicks since it's not obvious you are clicking on something.

> ::: toolkit/themes/osx/global/media/videocontrols.css
> @@ +48,5 @@
> >  }
> >  
> > +.controlBar[size="small"] > .playButton {
> > +  margin-right: 0;
> > +}
> 
> Hmm, was this broken for small-dimension <video>? *tests* oh dear. Did we
> goof this from the beginning, or did something else make this regress?
> 
> It's resetting the negative-margin used for the scrubber overlap, which has
> basically always been there... :|

We didn't goof this from the beginning. The change above from visibility:hidden to visibility:collapse makes this change necessary, but it now means that we can have the play/pause and mute buttons right next to each other.

Comment 46

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

Comment on attachment 759349 [details] [diff] [review]
Patch v1.1

Re-requesting review. Please see my response in comment #45.

Comment 47

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

Review ping?

Comment 48

[Justin Dolske [:Dolske]]

Attachment: Screenshot of Patch v.1.1

Something's still not right here. (Window on top is with patch, background is Nightly, video is 70 pixels wide)

Comment 49

[Justin Dolske [:Dolske]]

Comment on attachment 759349 [details] [diff] [review]
Patch v1.1

I think I'm ok with previous explanations. But the visual appearance is still goofy, so let's just get it sane.

Better testcase: http://dolske.net/mozilla/tests/video/sizes.html

Comment 50

[Justin Dolske [:Dolske]]

Updated:

http://dolske.net/mozilla/tests/video/sizes_audio.html
http://dolske.net/mozilla/tests/video/sizes_video.html

Also just noticed http://panuhorsmalahti.fi/audiodimensions.html was already listed here, that's useful too.

I'm now getting empty/tiny audio elements though. O_o

Comment 51

[Preeti[:preeti]]

Hello everyone! I am new to Bugzilla and this would be my first  mentored bug. I have little knowledge of python, HTML and CSS. In case, no one is working on this bug, may I please pick this up?
It would also  be great if someone please explains me the working procedure in case this bug is assigned to me.
Thanks in advance!

Comment 52

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

Thanks for your interest Preeti! Looking at the links in comment 50, this bug appears to have been fixed in the past five years.

I did some searching and it appears that it was fixed by these two bugs:

The first part is https://bugzilla.mozilla.org/show_bug.cgi?id=1271765 and the second part is a patch within this range, https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9af23c413a1f8d337b19b4f8450e241e91b71136&tochange=587daa4bdc4b40b7053f4ca3b74723ca747f3b52

Bug 1271765 mainly fixed this, though without the second part audio elements that were very narrow weren't getting rendered.

Comment 53

[Preeti[:preeti]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #52)
> Thanks for your interest Preeti! Looking at the links in comment 50, this
> bug appears to have been fixed in the past five years.
> 
> I did some searching and it appears that it was fixed by these two bugs:
> 
> The first part is https://bugzilla.mozilla.org/show_bug.cgi?id=1271765 and
> the second part is a patch within this range,
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=9af23c413a1f8d337b19b4f8450e241e91b71136&tochange=587d
> aa4bdc4b40b7053f4ca3b74723ca747f3b52
> 
> Bug 1271765 mainly fixed this, though without the second part audio elements
> that were very narrow weren't getting rendered.

Ok, Thank you!