1059726 - Hide Controls/Show Controls displays fullscreen button for audio files.

Status: RESOLVED DUPLICATE of bug 1222669

(Toolkit :: Video/Audio Controls, defect)

Description

[Catalin Varga [QA][:VarCat]]

Environment:
Firefox 31.1 ESR:
Build Id: 20140825060242
OS: Win 8 x32, Mac Os X 10.8.5, Ubuntu 14.04 x64


STR:

1. Open http://www.texasbagpiper.com/soundclips/11%20-%20Track%2011.mp3
2. Right Click Hide Controls.
3. Right Click Show Controls.

Issue:
Fullscreen button is being displayed for an audio file.
Pressing the fullscreen button will make the window go fullscreen but the controls will disappear.

The bug is reproducible for FF 32, FF 33 and FF 34.

A regression-window will be provided tomorrow.

Comment 1

[Catalin Varga [QA][:VarCat]]

This is not a regression this seems more like a partial fix.

The first revision with this behavior is  79b5c74ef97b (2013-08-07)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1e381c91885d&tochange=79b5c74ef97b

Previously to this revision after pressing Hide Controls the controls cannot be brought back.

Comment 2

[Ben Craddock]

Can someone assign me this bug, please?  I don't yet have perms.

I can't send a patch right this instant because I can't compile to test it, but someone can go ahead and comment on my plan if they want.

I'm planning to remove "Hide controls" from the audio element context menu.  There are more problems with letting audio controls be hidden than just strange stuff like an option for fullscreen popping up.  For instance, if you hide the controls twice, everything disappears, including any chance to pull up the context menu again.

This all probably because the real bug is that you can click the hidden element at all.  If it's just supposed to be removing the control attribute, then there definitely shouldn't be anything to right click after that (just like any <audio src=...> element).

I think this is a good opportunity to remove this confusing option.  If you hide the controls on a video at the bottom of a long page, scroll up, then decide you want to unhide them and scroll back down, you can see the video to right click and do that.  If you try that with an audio element, you can't see anything to right click, since the only thing to see in the first place was the control bar.  Avoiding the chance to get stuck hunting for an invisible element is probably a good thing.

I also just tested Chrome, IE, and Opera.  None of them let the user hide audio controls.

Comment 3

[Ben Craddock]

Attachment: patch1 - hide "Hide Controls" for <audio>

Here's the patch for the above-mentioned menu hiding.

Comment 4

[Ben Craddock]

Attachment: patch2 - Hide "Hide Controls" for <audio> (w/ commit msg)

Oops, forgot the commit message.

Comment 5

[Ben Craddock]

I just noticed that the unmute option is disabled, not hidden, for videos with no audio.  So it might be more consistent with the way the context menu already works to set "Hide Controls" to disabled as well for audio controls.  That's another possibility.

I'm also willing to just "fix the bug" if I could get some direction for what should actually be the intended behavior.  For instance, if "Hide Controls" removes the control attribute from the element, then the <audio> element won't have a container on the page to right click.  So hiding audio controls would remove the container from the page and confuse users.  However, the alternative is an invisible element, which doesn't make much sense to me either.

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 8500938 [details] [diff] [review]
patch2 - Hide "Hide Controls" for <audio> (w/ commit msg)

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

Sorry for taking so long. I think this is OK, but an incomplete fix. Could you take a look at setupInitialState() to see if we can fix that?

::: browser/base/content/nsContextMenu.js
@@ +456,5 @@
>      this.showItem("context-media-mute",   onMedia && !this.target.muted);
>      this.showItem("context-media-unmute", onMedia && this.target.muted);
>      this.showItem("context-media-playbackrate", onMedia);
> +    this.showItem("context-media-showcontrols", this.onVideo && !this.target.controls);
> +    this.showItem("context-media-hidecontrols", this.onVideo && this.target.controls);

Hmm. I seem to recall this once having had some value because it's easy to forget the |controls| attribute, and that <audio> used to (?) take up some default space in the page like <video> (ie, you had something to right-click on).

That doesn't seem terribly useful now. <audio> doesn't take up space by default, so this would only be useful if a page explicitly gave it a size but forgot to set |controls|. Seems like an edge case. So a context menu for it isn't super useful, especially given that Chrome/IE/Opera have done it.

But, technically this doesn't fix the bug, since the page itself could still add/remove the controls attribute. (Although it does remove a user footgun.)

Seems like it should be possible to fix this -- what's videocontrols.xml's setupInitialState() doing when the fullscreen button is shown? If that's a straightforward fix, we should do it too.

Comment 7

[Justin Dolske [:Dolske]]

(In reply to Ben Craddock from comment #5)
> I just noticed that the unmute option is disabled, not hidden, for videos
> with no audio.  So it might be more consistent with the way the context menu
> already works to set "Hide Controls" to disabled as well for audio controls.
> That's another possibility.

The general principle is that it's confusing to have menuitems that appear and disappear randomly (if the user isn't likely understand what's actually happening, which is often the case). And so it's better to disable an item to indicate it isn't available, instead of removing it. It also serves as an indication that a normal operation isn't currently available.

I think it makes sense to remove Show/Hide Controls from <audio>, though, since they're just not relevant to <audio> elements now.

Comment 8

[Ada [:adalucinet]]

Issue still reproducible with STR from comment 0 and Firefox 34 beta 10 (Build ID: 20141117202603) on Windows 7 64-bit, Windows 8 32-bit, Mac OS X 10.9.5 and Ubuntu 12.04 32-bit.

Comment 9

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

Fixed by bug 1222669.