138074 - context menu for Image as URL doesn't match spec

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[sairuh (rarely reading bugmail)]

realized that bug 136986 is invalid. the issue here is that the "image as url"
context menu has not been implemented. what we get now is the menu for "inline
image", when the pointer is over the image, and the menu for the regular content
area when pointer is not over the image.

the spec'd context menu is at
http://www.mozilla.org/projects/ui/communicator/framework/contextmenus/#Anchor-7191
and would include the following:

Back
Forward
Reload
Stop
[sep]
Copy Image (see bug 135300)
Copy Image Location
Bookmark This Page
[sep]
Save Image
Send Image
Send to Buddy (commercial-only rfe)
Set as Wallpaper (win32-only for the time being)
[sep]
Image Properties

Comment 1

[sairuh (rarely reading bugmail)]

tentatively nominating (unsure about the time available, etc)...

Comment 2

[Paul Wyskoczka]

Per the Nav triage team marking nsbeta1+ [adt2 rtm]

Comment 3

[Blake Ross]

What *is* an Image as URL?  As opposed to an Image as Link?  The only think I
can think of is that it's the context menu for an image when you just load the
image, e.g. http://wwww.webaddress.com/foo.gif.  If so, I can't really
understand how this could be critical for rtm, but...

Comment 4

[sairuh (rarely reading bugmail)]

yep, the context menu for "image as url" is for when something like
http://mozilla.org/images/mozilla-banner.gif is loaded for the page's url.

Comment 5

[Peter Trudelle]

I agree with Blake, why is this critical?  removing plus for retriage.

Comment 6

[Peter Trudelle]

nsbeta1- per Nav triage team.

Comment 7

[sairuh (rarely reading bugmail)]

nominating for buffy.

Comment 8

[timeless]

Attachment: remove send page, save page, view image, and /optionally/ bookmark this page

currently
! view image 
  block images from this server
  copy image location
  -
  save image as
  send image
  set as wallpaper
  -
? bookmark this page
! save page as
! send page
  copy
  select all
  -
  properties

The three items i've marked with ! are really inappropriate. The item I marked
with ? is questionable. This patch removes all four. I'll take a review from
xpfe specifying what to do with ? (it's easy to just not commit that line).

Comment 9

[jag (Peter Annema)]

I'd say keep "bookmark this page", you're right on the other three though. Fix
those two logic errors I pointed out on #mozilla and sr=jag

Comment 10

[timeless]

Attachment: same as before but without a logic glitch

Comment 11

[jag (Peter Annema)]

Comment on attachment 104049 [details] [diff] [review]
same as before but without a logic glitch

sr=jag

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 104049 [details] [diff] [review]
same as before but without a logic glitch

Shouldn't you call |isImage| something more distinct from |onImage| so people
don't confuse them?  How about |onImageDoc|?

Comment 13

[Asa Dotzler [:asa]]

This can wait until 1.3 opens (real soon now). It's not a recent regression,
major crash or near-zero risk fix so please hold off on landing until 1.3a
opens. Thanks.

Comment 14

[Paul Wyskoczka]

nsbeta1- per the nav triage team.

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 104049 [details] [diff] [review]
same as before but without a logic glitch

I agree with dbaron.  This will work even if you are right clicking on the page
outside of the image in the image doc, right?  So 'isImage' may not be entirely
accurate.  r=caillon if you use onImageDoc

Comment 16

[Vedran Miletic]

retargeting

Comment 17

[Boris Zbarsky [:bzbarsky]]

To timeless to maybe check in the patch, seeing as it has reviews....

Comment 18

[Phil Ringnalda (:philor)]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsContextMenu.js&branch=1.125&root=/cvsroot&subdir=mozilla/suite/common&command=DIFF_FRAMESET&rev1=1.84&rev2=1.85 - fixed a bit over five years ago.