496832 - Disable media entries in context menu when media source is invalid

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Pascal Chevrel:pascalc (PTO until April 26)]

1/ Go to the test case attached to this bug. It contains an image with a wrong link, a video with a wrong link and a video with a correct link.

2/ right click on the second square which is the video with the wrong link, select 'copy url"

Expected result:
Same behaviour as for the image tag, the url is copied to the clipboard

Actual result:
Nothing is copied, this feature works only if the url is correct

Comment 1

[Pascal Chevrel:pascalc (PTO until April 26)]

Attachment: testcase

Comment 2

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090607 Minefield/3.6a1pre

Regression range is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=884a5c12182a&tochange=73c3f66ec28f

Comment 3

[Matthew Gregan [:kinetik]]

The context menu uses the element's currentSrc attribute when copying the URL.  This attribute is only set after a valid media source has selected, so you can't rely on the value of it before receiving the first progress event (not sure if anything more appropriate fires when currentSrc becomes valid, a quick skim of the spec didn't reveal anything), nor when the resource selection algorithm fails to find a valid media source.

Using currentSrc is the correct thing to do once it becomes valid, since the media element may have selected any one of a number of possible sources.

It's not clear to me what the right behaviour is when currentSrc isn't valid.  Maybe we should just disable the "Copy URL" menu item?

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

When the URL is pointing to a faulty resource, copying the URL becomes a diagnostic sort of thing. This doesn't block the release of Firefox 3.5.

As for comment 3, if we can't actually make the menuItem do anything here, then yes, it should be disabled.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

This disables various context menu media entries when there are problems.

1) The entries that deal with a URL (copy/send/view) are disabled when no URL is available. I also added a fallback to use .src when .currentSrc isn't set... This allows using these commands on <video src="bad.avi">. I don't *think* it's possible to have to have .src and <source> trying to load different things... The spec seems to say that the src attribute should override any <source> that might be present. I think the only time this could even matter, though, is early on in the initial load.

2) The entries that deal with controlling the media (play/mute/show-controls) are disabled when the video is in an error state. I'm a little unsure of how non-recoverable errors are (eg, should "play" restart a video that ended because it was corrupted?), but I guess we already hide the controls when an error happens, so it's consistent with that.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 383219 [details] [diff] [review]
Patch v.1

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>+    if (onMedia) {
>+      this.setItemAttr("context-media-play",  "disabled", (this.target.error != null));

Use a temporary for (error != null)?

>-        this.mediaURL = this.target.currentSrc;
>+        this.mediaURL = this.target.currentSrc || this.target.src;

When is currentSrc empty but src not, out of curiosity?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> When is currentSrc empty but src not, out of curiosity?

Nevermind, re-read comment 5.

We should be able to test this right?

Comment 10

[Justin Dolske [:Dolske]]

Added tests and fixed nits.

Pushed http://hg.mozilla.org/mozilla-central/rev/d420c721e863

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch v.2 (checked in)

I'd like to land this in 3.5.1. Safe and understood patch, context menu has existing tests and added more for this specific change.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 387793 [details] [diff] [review]
Patch v.2 (checked in)

a1912=beltzner

Comment 13

[Justin Dolske [:Dolske]]

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/584af10f1117

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk and 1.9.1 with builds on all platforms like:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090722 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090722042136

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2pre) Gecko/20090729 Shiretoko/3.5.2pre (.NET CLR 3.5.30729) ID:20090729045022