Last Comment Bug 729633 - Disable "View Background Image" in context menu for stand-alone video content (SeaMonkey)
: Disable "View Background Image" in context menu for stand-alone video content...
Status: RESOLVED FIXED
: ux-error-prevention
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: seamonkey2.10
Assigned To: rsx11m
:
Mentors:
Depends on: 713173
Blocks: 715681
  Show dependency treegraph
 
Reported: 2012-02-22 11:20 PST by rsx11m
Modified: 2012-03-01 10:36 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
fixed
fixed


Attachments
Proposed fix (1.24 KB, patch)
2012-02-27 07:38 PST, rsx11m
no flags Details | Diff | Review
Proposed fix (v2) (2.44 KB, patch)
2012-02-27 14:50 PST, rsx11m
no flags Details | Diff | Review
Proposed fix (v3) [Checkin: Comments 16 and 19] (2.21 KB, patch)
2012-02-27 16:42 PST, rsx11m
neil: review+
bugspam.Callek: approval‑comm‑aurora+
Details | Diff | Review

Description rsx11m 2012-02-22 11:20:04 PST
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).
Comment 1 Philip Chee 2012-02-23 03:21:25 PST
This also happens in Firefox.
Comment 2 rsx11m 2012-02-23 06:26:59 PST
Thanks for double-checking. Shall we try to work around it in SeaMonkey or rather move this bug into a Core or Toolkit component? I didn't find anything in their component descriptions that fully matches though, especially given that it could be solved either in the themes or context-menu implementation for this content.
Comment 3 Philip Chee 2012-02-23 10:28:33 PST
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.
Comment 4 rsx11m 2012-02-23 11:23:00 PST
I don't see any immediate fix when looking at the SeaMonkey implementation of nsContextMenu, specifically where to get the context from that it's a stand-alone video content, thus I've filed this for Firefox in parallel as suggested (bug 730041).
Comment 5 Philip Chee 2012-02-26 20:13:11 PST
> specifically where to get the context from that it's a stand-alone video content,

<http://mxr.mozilla.org/comm-central/source/mozilla/dom/interfaces/core/nsIDOMDocument.idl#313>

  /**
   * True if this document is synthetic : stand alone image, video, audio file,
   * etc.
   */
  readonly attribute boolean        mozSyntheticDocument;

Example:
<http://mxr.mozilla.org/comm-central/source/suite/common/nsContextMenu.js#508>

    // Check if we are in a synthetic document (stand alone image, video, etc.).
    this.inSyntheticDoc = this.target.ownerDocument.mozSyntheticDocument;

HTH
Comment 6 rsx11m 2012-02-27 07:38:11 PST
Created attachment 600904 [details] [diff] [review]
Proposed fix

Thanks Phil, that helped a lot - and there is actually a respective comment right above the line you referred to which (how embarrassing!) I've missed.

So, the stand-alone video apparently hits the default rule at the very bottom, thus checking there for a synthetic document prevents the "View Background Image" menu item to appear.

I've tested this with various HTML pages that have a background image either at the top <BODY> level or somewhere within the page, not seeing any other images excluded from being selectable, thus it looks like there are no side effects.
Comment 7 neil@parkwaycc.co.uk 2012-02-27 08:17:02 PST
Apparently the image is the same one as tabprompts-bgtexture.png from toolkit.
Comment 8 neil@parkwaycc.co.uk 2012-02-27 08:30:24 PST
Comment on attachment 600904 [details] [diff] [review]
Proposed fix

Wouldn't it make more sense to tweak lines 230/1 instead?
Comment 9 rsx11m 2012-02-27 09:06:16 PST
(In reply to neil@parkwaycc.co.uk from comment #7)
> Apparently the image is the same one as tabprompts-bgtexture.png from toolkit.

Yes, that's actually what's showing up in current 2.7.2 and earlier versions.

(In reply to neil@parkwaycc.co.uk from comment #8)
> Wouldn't it make more sense to tweak lines 230/1 instead?

Looking at it I'm not sure as it would apply to all synthetic documents whereas the current patch catches the case where it's actually needed (i.e., no lower-level background images). Thus, I'm wondering if there are any variants of a synthetic document where you may have a valid background image which we would prevent from being accessible in a more general fix.

BTW: There are various cases where "View Background Image" is present but disabled depending on the content, thus I figured that just disabling it is equally sufficient as hiding.

Anyway, I can investigate the general hiding option later if that's the preferred solution.
Comment 10 rsx11m 2012-02-27 11:47:33 PST
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.
Comment 11 rsx11m 2012-02-27 14:50:19 PST
Created attachment 601079 [details] [diff] [review]
Proposed fix (v2)

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.
Comment 12 neil@parkwaycc.co.uk 2012-02-27 15:46:36 PST
Comment on attachment 601079 [details] [diff] [review]
Proposed fix (v2)

>+                                         !this.onStandaloneImage &&
>+                                         !this.inSyntheticDoc);
I think you'll find that all standalone images are synthetic documents, so you don't need to do both checks (and you therefore don't have to wrap the line).

I like the idea of removing Select All though, it makes no sense and I can't see how you can unselect all once you've done it...
Comment 13 rsx11m 2012-02-27 16:42:40 PST
Created attachment 601126 [details] [diff] [review]
Proposed fix (v3) [Checkin: Comments 16 and 19]

(In reply to neil@parkwaycc.co.uk from comment #12)
> I think you'll find that all standalone images are synthetic documents, so
> you don't need to do both checks

Makes sense intuitively, but I wasn't sure that this is always the case.
Comment 14 neil@parkwaycc.co.uk 2012-02-28 01:21:20 PST
Comment on attachment 601126 [details] [diff] [review]
Proposed fix (v3) [Checkin: Comments 16 and 19]

Rather conveniently, we have no tests for context menus in synthetic documents.
Comment 15 rsx11m 2012-02-28 05:12:14 PST
Thanks Neil. Assuming that no further reviews are required, push for trunk please.
Comment 16 Jens Hatlak (:InvisibleSmiley) 2012-02-28 14:58:06 PST
Comment on attachment 601126 [details] [diff] [review]
Proposed fix (v3) [Checkin: Comments 16 and 19]

http://hg.mozilla.org/comm-central/rev/5445b9cf8c2a
Comment 17 rsx11m 2012-02-29 10:13:09 PST
Comment on attachment 601126 [details] [diff] [review]
Proposed fix (v3) [Checkin: Comments 16 and 19]

[Approval Request Comment]
Despite the Toolkit style changes becoming effective with SM 2.8 already, this fix won't qualify for the beta channel as this.inSyntheticDoc was introduced with bug 713173 and becomes available with SM 2.9 only.

User impact if declined: This is mostly to avoid user confusion by accidentally invoking functions in the context menu causing surprising actions and making it difficult to return to the previous "good" state.
 
Testing completed (on c-c, etc.): Tinderboxes look ok on trunk, tested on aurora build and works fine there.

Risk to taking this patch (and alternatives if risky): low.

String changes made by this patch: none.
Comment 18 rsx11m 2012-03-01 06:18:02 PST
Thanks Callek, push for comm-aurora please.
Comment 19 Jens Hatlak (:InvisibleSmiley) 2012-03-01 10:36:33 PST
Comment on attachment 601126 [details] [diff] [review]
Proposed fix (v3) [Checkin: Comments 16 and 19]

http://hg.mozilla.org/releases/comm-aurora/rev/5afefec96bc1

Note You need to log in before you can comment on or make changes to this bug.