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.
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).
> 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
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.
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 firstname.lastname@example.org 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 email@example.com 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.
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 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...
Created attachment 601126 [details] [diff] [review] Proposed fix (v3) [Checkin: Comments 16 and 19] (In reply to firstname.lastname@example.org 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 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.
Thanks Neil. Assuming that no further reviews are required, push for trunk please.
Comment on attachment 601126 [details] [diff] [review] Proposed fix (v3) [Checkin: Comments 16 and 19] http://hg.mozilla.org/comm-central/rev/5445b9cf8c2a
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.
Thanks Callek, push for comm-aurora please.
Comment on attachment 601126 [details] [diff] [review] Proposed fix (v3) [Checkin: Comments 16 and 19] http://hg.mozilla.org/releases/comm-aurora/rev/5afefec96bc1