Open Bug 384854 Opened 13 years ago Updated 6 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

People

(Reporter: ehsan, Unassigned)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 4 obsolete files)

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
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 3 alpha6
Version: unspecified → Trunk
Assignee: nobody → ehsan.akhgari
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Attached file Testcase (obsolete) —
Try right-clicking in the left frame, and from the This Frame menu item, choose View Frame Source.
Summary: Disable certain context menu options (view source, view selection sort, etc) when viewing non-text content (images, plugins) → Disable certain context menu options (view source, view selection source, etc) when viewing non-text content (images, plugins)
Attached patch Patch (obsolete) — Splinter Review
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>.
Attachment #268769 - Flags: review?(gavin.sharp)
Blocks: 206176
Maybe this should be blocked by bug 359057.
Could we get a review on this before alpha6 freeze?  Otherwise we have to retarget it to the next milestone.
Retargetting to beta1.
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
also image/svg has a text source and should be added to mimeTypeIsTextBased(aMimeType)
ops. sorry, don't mind, it's already included in +xml
Comment on attachment 268769 [details] [diff] [review]
Patch

Requesting review from mano...
Attachment #268769 - Flags: review?(gavin.sharp) → review?(mano)
Comment on attachment 268769 [details] [diff] [review]
Patch

beltzer says "hide", not disable.
Attachment #268769 - Flags: review?(mano) → review-
Editing the summary based on comment 9.
Summary: Disable certain context menu options (view source, view selection source, etc) when viewing non-text content (images, plugins) → Hide certain context menu options (view source, view selection source, etc) when viewing non-text content (images, plugins)
(In reply to comment #9)
> (From update of attachment 268769 [details] [diff] [review])
> beltzer says "hide", not disable.

Done.
Attachment #268769 - Attachment is obsolete: true
Attachment #274973 - Flags: review?(mano)
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.
Attachment #274973 - Flags: review?(mano) → review+
Attached patch Patch for checkin (obsolete) — Splinter Review
Patch for checkin with notes in comment 12 addressed.
Marking this for checkin...
Keywords: checkin-needed
Target Milestone: Firefox 3 M7 → Firefox 3 M8
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Keywords: verifyme
Blocks: 402246
Flags: in-litmus?
Keywords: verifyme
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?
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch New patch + testSplinter Review
This patch corrects the hiding logic and also provides an automated test, so the Litmus test will no longer be necessary.
Attachment #268763 - Attachment is obsolete: true
Attachment #274973 - Attachment is obsolete: true
Attachment #279235 - Attachment is obsolete: true
Attachment #385384 - Flags: review?(mano)
Flags: in-litmus? → in-testsuite?
Status: REOPENED → ASSIGNED
Updating to reality: I won't have the time to work on this for the foreseeable future!
Assignee: ehsan.akhgari → nobody
Status: ASSIGNED → NEW
Attachment #385384 - Flags: review?(mano)
Target Milestone: Firefox 3 alpha8 → ---
You need to log in before you can comment on or make changes to this bug.