1222669 - Standalone audio looks broken after hiding controls

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[arni2033]

STR
1. Open the URL (from bug 1222374) or any other local audio file
>  https://dl.dropboxusercontent.com/s/k3i4yuc7j3s93hb/bug%20audio%20box%20shadow%20-%20donut%20quest.mp3?dl=0
2. Right-click the audio, click "Hide controls"   [you'll get the empty rectangle with box-shadow]
3. Right-click the empty rectangle, click "Show controls"
4. Right-click the audio, click "Hide controls"

Actual Results:
 After Step 3 <video> element has height == 0px, box-shadow isn't displayed
 After Step 4 it is not possible to right-click on the <video>, because it has height == 0px
              so you can't bring back the video controls.

Regression window:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=c17073bda80b&tochange=f9a5108cf2b2

Regressed by:
 f9a5108cf2b2   Jared Wein — Bug 704326 - Standalone audio files should have different style so they
                don't look awkward. r=smaug,dolske

Comment 1

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

Attachment: MozReview Request: Bug 1222669 - Don't show the hidecontrols contextmenu item when viewing a standalone audio file. r?gijs

Review commit: https://reviewboard.mozilla.org/r/45525/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45525/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8740058 [details]
MozReview Request: Bug 1222669 - Don't show the hidecontrols contextmenu item when viewing a standalone audio file. r?gijs

https://reviewboard.mozilla.org/r/45525/#review42067

::: browser/base/content/nsContextMenu.js:475
(Diff revision 1)
>      this.showItem("context-media-pause", onMedia && !this.target.paused && !this.target.ended);
>      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", onMedia && !this.target.controls);
> -    this.showItem("context-media-hidecontrols", onMedia && this.target.controls);
> +    this.showItem("context-media-hidecontrols", this.target.controls && (this.onVideo || (this.onAudio && !this.inSyntheticDoc)));

Is the explicit check for onAudio useful here? I'm assuming that onAudio == !onVideo at all times. That is, couldn't this just be:

```
(this.onVideo || !this.inSyntheticDoc)
```

?

::: browser/base/content/test/general/browser_contextmenu.js:36
(Diff revision 1)
> +
> +    let audioIframe = doc.querySelector("#test-audio-in-iframe");
> +    // media documents always use a <video> tag.
> +    let audio = audioIframe.contentDocument.querySelector("video");
> +    audio.pause();
> +    yield ContentTaskUtils.waitForCondition(() => audio.paused, "iframe audio should be paused");

Is there not an event you can wait for instead?

Comment 3

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

https://reviewboard.mozilla.org/r/45525/#review42067

> Is the explicit check for onAudio useful here? I'm assuming that onAudio == !onVideo at all times. That is, couldn't this just be:
> 
> ```
> (this.onVideo || !this.inSyntheticDoc)
> ```
> 
> ?

Well, we don't want to show the menuitem for non-media elements. `data:text/html,<p controls=controls>this is some text</p>` shouldn't show a hideControls menuitem.

So, if we qualify that with knowing that it is onMedia, then yes, they should be mutually exclusive, but that extra qualification isn't necessary if we write it this way.

> Is there not an event you can wait for instead?

Yeah, we could wait for the pause event.

Comment 4

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

Attachment: Patch for check-in

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/80fa4e428fb2

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/80fa4e428fb2

Comment 8

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Sounds to me like this could also fix the case in bug 1010976. Setting for a quick verification.

Comment 9

[Camelia Badau [:cbadau], Desktop QA]

I've reproduced the initial issue on Nightly 06-11-2016. 

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.11.1 using Firefox 48 Beta 2 (buildID: 20160620091522).