Closed Bug 730041 Opened 12 years ago Closed 12 years ago

Disable "View Background Image" in context menu for stand-alone video content (Firefox)

Categories

(Firefox :: Menus, defect)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

Details

(Keywords: ux-error-prevention)

Attachments

(1 file, 1 obsolete file)

Originally filed for SeaMonkey:

+++ This bug was initially created as a clone of Bug #729633 +++

Recently the layout for displaying stand-alone images or videos has been changed (likely in Toolkit, so maybe some change was missing that SeaMonkey yet needs to port), showing the content now centered on dark background.

For videos, the background contains a texture which is treated as a background image and coincidentally can be viewed in isolation. The suggestion is (similar to the disabled "Page Info" menu item) to disable "View Background Image" in the context menu in this mode.

Steps to reproduce:
 - Open an OGG or WebM video file or stream
 - place cursor outside the video content
 - right-click to get the context menu
 - select "View Background Image"
 - video stops, a small texture image is displayed
 - location bar shows "data:image/png..."

Not a big deal, but irritating if hit accidentally. As a workaround, the background image could simply be overridden by the themes, also given that it's apparently not used when viewing stand-alone images (inconsistent).

(Quoting Philip Chee from bug 729633 comment #1)
> This also happens in Firefox.

(Quoting Philip Chee from bug 729633 comment #3)
> File separate bug in Firefox (this needs to be fixed in the nsContextMenu
> implementation) and then copy their fix OR fix it here and backport it to
> Firefox. Your choice.

I don't see any immediate fix when looking at the SeaMonkey implementation, specifically where to get the context from that it's a stand-alone video content, thus filing for Firefox in parallel as suggested.
Blocks: 729633
No longer depends on: 729633
No longer blocks: 729633
Attached patch Port from SeaMonkey bug 729633 (obsolete) — Splinter Review
This is an adaption of attachment 601126 [details] [diff] [review], hiding the "View Background Image" and "Select All" context-menu items in a stand-alone synthetic document.

Below are the relevant justifications from the other bug:

(Quoting rsx11m from bug 729633 comment #10)
> Handling it in the initViewItems() function instead should be safe given
> that MediaDocument::CreateSyntheticDocument() is the only place setting this
> attribute and only used for stand-alone video/audio, image, or plugin pages.
> 
> > http://mxr.mozilla.org/mozilla-central/ident?i=CreateSyntheticDocument
> 
> Thus, there shouldn't be any lower-level background images in such documents
> that need to be accessible. I'm wondering if "Select All" should be removed
> from the context menu as well in these cases as the only content is the
> media itself and clipboard operations should be handled by the respective
> context menu controls over the content.

(Quoting rsx11m from bug 729633 comment #11)
> 
> Rather than just disabling "View Background Image" this patch removes/hides
> it in a synthetic document per Neil's suggestion. It also hides "Select All"
> in the context menu which doesn't make sense in this type of content (it's
> still in the "Edit" menu if someone desperately wants it). This directs the
> user to the image or video itself to select "Copy Image" or "Copy Video
> Location" there, thus also avoids accidentally putting the HTML structure of
> the synthetic document onto the clipboard with a "Select All" then "Copy"
> sequence.

I may not know the Firefox review rules good enough but asking Justin for review, given that he seems to have reviewed many video-related patches for this file in the past. I won't be able to add or modify any tests if such are necessary.
Attachment #601666 - Flags: review?(dolske)
Comment on attachment 601666 [details] [diff] [review]
Port from SeaMonkey bug 729633

>     this.showItem("context-selectall", !(this.onLink || this.onImage ||
>-                  this.onVideo || this.onAudio) || this.isDesignMode);
>+                  this.onVideo || this.onAudio) || this.isDesignMode ||
>+                  this.inSyntheticDoc);

This is broken, you don't want to show the item for synthetic docs.
(In reply to Dão Gottwald [:dao] from comment #2)
> Comment on attachment 601666 [details] [diff] [review]
> Port from SeaMonkey bug 729633
> 
> >     this.showItem("context-selectall", !(this.onLink || this.onImage ||
> >-                  this.onVideo || this.onAudio) || this.isDesignMode);
> >+                  this.onVideo || this.onAudio) || this.isDesignMode ||
> >+                  this.inSyntheticDoc);
> 
> This is broken, you don't want to show the item for synthetic docs.

Hmm? It's in a !(a||b||c) group, so it seems right to me?
Comment on attachment 601666 [details] [diff] [review]
Port from SeaMonkey bug 729633

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

Marking r+ unless I missed something obvious in the last comment. :)

Please check 2 things before landing:

1) Any changes to test_contextmenu.html needed? (Ie, does this change something the test's expecting)

2) Is the extra separator being removed? Right now the menu looks like:

  ...
  -------------
  View BG Image
  Select All
  -------------
  ...

With these two items removed, we don't want to end up with 2 sequential menuseperators.
Attachment #601666 - Flags: review?(dolske) → review+
I'd agree with Dão that this.inSyntheticDoc should be in the !(...) context, as posted it's outside of that context. I'll double-check if the patch is what I've actually tested and will provide an updated patch if necessary.

> 2) Is the extra separator being removed?

The patch removes the "View BG Image" separator, thus leaving the one below "Select All" whether or not that part is removed.
You'te missing the closing ')'.
It's "this.showItem ( , ! ( ... ) || );", thus the parentheses should be fine, I'll only have to move the test for the synthetic document back into that clause. Looks like a copy-paste error from the SM patch which didn't have this.isDesignMode, but I'll retest that to be sure.

(In reply to Justin Dolske [:Dolske] from comment #4)
> 1) Any changes to test_contextmenu.html needed? (Ie, does this change
> something the test's expecting)

While there are tests for the video context menu itself, I don't see any tests referring to a stand-alone media document with the context menu outside the content to be presented, thus I'd assume that the patch won't break anything.
Ok, this works now as tested with Firefox 13.0 and properly hides "Select All" as well. I've also verified that no double menuseparators are shown.

To make the nested-parentheses structure more intuitve, I've cleaned up white spaces and indentation a bit from its previous version, that hopefully helps.

Since everything in comment #4 should be addressed, carrying forward r=dolske from attachment 601666 [details] [diff] [review].
Assignee: nobody → rsx11m.pub
Attachment #601666 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #601730 - Flags: review+
Push for trunk, please (unless anybody objects).
Keywords: checkin-needed
Summary: Disable "View Background Image" in context menu for stand-alone video content → Disable "View Background Image" in context menu for stand-alone video content (Firefox)
Comment on attachment 601730 [details] [diff] [review]
Port from SeaMonkey bug 729633 (v2)

The first patch was broken, please get this re-reviewed.
Attachment #601730 - Flags: review+
Keywords: checkin-needed
Comment on attachment 601730 [details] [diff] [review]
Port from SeaMonkey bug 729633 (v2)

Thanks Dão for the clarification, I assumed r+ with all concerns resolved.
Attachment #601730 - Flags: review?(dolske)
(In reply to Dão Gottwald [:dao] from comment #6)
> You'te missing the closing ')'.

And this is why I shouldn't review in the morning. >_<
Attachment #601730 - Flags: review?(dolske) → review+
Sorry for the confusion (and for the glitch in the first patch).
Trying this again, now push for trunk please.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b394e82f30

Also, to make life easier for the people checking your patches in for you, please follow the below directions for future patches you submit. Thanks!
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d2b394e82f30
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: