384854 - Hide certain context menu options (view source, view selection source, etc) when viewing non-text content (images, plugins)

Status: NEW

(Firefox :: Menus, defect)

Description

[(no longer active)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070617 Minefield/3.0a6pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070617 Minefield/3.0a6pre

Bug 294383 disabled this stuff in Firefox application menus, but apparently left alone the corresponding context menu entries.  These entries should be disabled in page context menus as well.  The same thing should be applied to the View Frame Source menu item in framed pages if the active frame's content is non-text.

Reproducible: Always

Steps to Reproduce:
1. Go to <https://bugzilla.mozilla.org/mozilla-banner.gif>.
2. Right click on an empty section of the page.
3. Select View Page Source from the context menu, for example.
Actual Results:  
View Page Source is enabled.

Expected Results:  
View Page Source must be disabled.

about:buildconfig

Build platform
target
i686-pc-cygwin

Build tools
Compiler 	Version 	Compiler flags
$(CYGWIN_WRAPPER) cl 	14.00.50727 	-TC -nologo -W3 -Gy -Fd$(PDBFILE)
$(CYGWIN_WRAPPER) cl 	14.00.50727 	-GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fd$(PDBFILE) -I/usr/X11R6/include

Configure arguments
--enable-application=browser --enable-application=browser --disable-debug --enable-optimize --enable-canvas --enable-svg --enable-xpctools --disable-tests --enable-places --enable-places-bookmarks --enable-storage --enable-safe-browsing --enable-url-classifier --enable-strip --disable-javaxpcom

Comment 1

[(no longer active)]

Attachment: Testcase

Try right-clicking in the left frame, and from the This Frame menu item, choose View Frame Source.

Comment 2

[(no longer active)]

Attachment: Patch

This disables View Source and View Selection Source, as well the This Frame -> View Frame Source menu items for non-text content.

This can be tested both on the testcase (attachment 268763 [details]) and the bug's URL <https://bugzilla.mozilla.org/mozilla-banner.gif>.

Comment 3

[[:Aleksej]]

Maybe this should be blocked by bug 359057.

Comment 4

[(no longer active)]

Could we get a review on this before alpha6 freeze?  Otherwise we have to retarget it to the next milestone.

Comment 5

[(no longer active)]

Retargetting to beta1.

Comment 6

[Marco Bonardo [:mak]]

also image/svg has a text source and should be added to mimeTypeIsTextBased(aMimeType)

Comment 7

[Marco Bonardo [:mak]]

ops. sorry, don't mind, it's already included in +xml

Comment 8

[(no longer active)]

Comment on attachment 268769 [details] [diff] [review]
Patch

Requesting review from mano...

Comment 9

[Asaf Romano (gone)]

Comment on attachment 268769 [details] [diff] [review]
Patch

beltzer says "hide", not disable.

Comment 10

[(no longer active)]

Editing the summary based on comment 9.

Comment 11

[(no longer active)]

Attachment: Patch revised to hide instead of disabling

(In reply to comment #9)
> (From update of attachment 268769 [details] [diff] [review])
> beltzer says "hide", not disable.

Done.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 274973 [details] [diff] [review]
Patch revised to hide instead of disabling


>Index: browser/base/content/browser.js
>===================================================================
>+/**
>+ * Returns true if |aMimeType| is text-based, or false otherwise.

s/or/

>Index: browser/base/content/nsContextMenu.js
>===================================================================

> 
> function nsContextMenu(aXulMenu, aBrowser) {
>   this.target            = null;
>   this.browser           = null;
>   this.menu              = null;
>+  this.isFrameImage      = null;

|false|


>   // Initialize context menu.
>   initMenu: function CM_initMenu(aPopup, aBrowser) {
>     this.menu = aPopup;
>     this.browser = aBrowser;
>+    

nit: trailing spaces.

r=mano otherwise.

Comment 13

[(no longer active)]

Attachment: Patch for checkin

Patch for checkin with notes in comment 12 addressed.

Comment 14

[(no longer active)]

Marking this for checkin...

Comment 15

[Nickolay_Ponomarev]

mozilla/browser/base/content/browser-context.inc  1.33
mozilla/browser/base/content/browser-sets.inc     1.102
mozilla/browser/base/content/browser.js           1.836
mozilla/browser/base/content/nsContextMenu.js     1.25

Comment 16

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

Ehsan, so it was meant to hide and not disable those context menu entries? Why the e.g. View Page Source is still visible but hidden when opening an image like http://www.google.de/intl/de_de/images/logo.gif?

Do we have a new regression?

Comment 17

[(no longer active)]

(In reply to comment #16)
> Ehsan, so it was meant to hide and not disable those context menu entries? Why
> the e.g. View Page Source is still visible but hidden when opening an image
> like http://www.google.de/intl/de_de/images/logo.gif?
> 
> Do we have a new regression?

No, in fact as it turns out, this was never fixed properly.  I have a patch to fix this in a correct way which I'll attach.

Comment 18

[(no longer active)]

Attachment: New patch + test

This patch corrects the hiding logic and also provides an automated test, so the Litmus test will no longer be necessary.

Comment 19

[(no longer active)]

Updating to reality: I won't have the time to work on this for the foreseeable future!