Closed Bug 1222669 Opened 4 years ago Closed 4 years ago

Standalone audio looks broken after hiding controls

Categories

(Toolkit :: Video/Audio Controls, defect, trivial)

defect
Not set
trivial

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- affected
firefox48 --- verified

People

(Reporter: arni2033, Assigned: jaws)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 1 obsolete file)

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
Flags: needinfo?(jaws)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
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?
Attachment #8740058 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Attachment #8740058 - Attachment is obsolete: true
Attachment #8740087 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/80fa4e428fb2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Duplicate of this bug: 1059726
Sounds to me like this could also fix the case in bug 1010976. Setting for a quick verification.
Flags: qe-verify+
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.