Closed Bug 729633 Opened 12 years ago Closed 12 years ago

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

Categories

(SeaMonkey :: UI Design, defect)

Product:
SeaMonkey
Component:
UI Design
Type:
defect
Priority:
Not set
Severity:
trivial

Tracking

(seamonkey2.8 wontfix, seamonkey2.9 fixed, seamonkey2.10 fixed)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.10
Tracking Flags:
Tracking Status
seamonkey2.8 --- wontfix
seamonkey2.9 --- fixed
seamonkey2.10 --- fixed

People

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

References

(Blocks 1 open bug)

Details

(Keywords: ux-error-prevention)

Attachments

(1 file, 2 obsolete files)

Proposed fix (v3) [Checkin: Comments 16 and 19]
2.21 KB, patch
neil
: review+
Callek
: approval-comm-aurora+
Details | Diff | Splinter Review 
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).
This also happens in Firefox.
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.
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.
Blocks: 730041
No longer blocks: 730041
Depends on: 730041
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).
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 (SeaMonkey)
> 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
Attached patch Proposed fix (obsolete) — — Splinter Review 
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.
Attachment #600904 - Flags: review?(neil)
Apparently the image is the same one as tabprompts-bgtexture.png from toolkit.
Comment on attachment 600904 [details] [diff] [review]
Proposed fix

Wouldn't it make more sense to tweak lines 230/1 instead?
(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.
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.
Attached patch Proposed fix (v2) (obsolete) — — Splinter Review 
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.
Attachment #600904 - Attachment is obsolete: true
Attachment #601079 - Flags: review?(neil)
Attachment #600904 - Flags: review?(neil)
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...
(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.
Attachment #601079 - Attachment is obsolete: true
Attachment #601126 - Flags: review?(neil)
Attachment #601079 - Flags: review?(neil)
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.
Attachment #601126 - Flags: review?(neil) → review+
Assignee: nobody → rsx11m.pub
Blocks: 715681
Status: NEW → ASSIGNED
Thanks Neil. Assuming that no further reviews are required, push for trunk please.
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
Comment on attachment 601126 [details] [diff] [review]
Proposed fix (v3) [Checkin: Comments 16 and 19]

http://hg.mozilla.org/comm-central/rev/5445b9cf8c2a
Attachment #601126 - Attachment description: Proposed fix (v3) → Proposed fix (v3) [Checkin: Comment 16]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → seamonkey2.10
status-seamonkey2.10: --- → fixed
status-seamonkey2.8: --- → wontfix
status-seamonkey2.9: --- → affected
Depends on: 713173
No longer depends on: 730041
Keywords: ux-error-prevention
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.
Attachment #601126 - Flags: approval-comm-aurora?
Attachment #601126 - Flags: approval-comm-aurora? → approval-comm-aurora+
Thanks Callek, push for comm-aurora please.
Keywords: checkin-needed
Whiteboard: [c-n: comm-aurora]
Comment on attachment 601126 [details] [diff] [review]
Proposed fix (v3) [Checkin: Comments 16 and 19]

http://hg.mozilla.org/releases/comm-aurora/rev/5afefec96bc1
Attachment #601126 - Attachment description: Proposed fix (v3) [Checkin: Comment 16] → Proposed fix (v3) [Checkin: Comments 16 and 19]
status-seamonkey2.9: affected → fixed
Keywords: checkin-needed
Whiteboard: [c-n: comm-aurora]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: