482941 - 'View Background Image' context-menu item is always greyed out

Status: RESOLVED FIXED

(Firefox :: Menus, defect, P2)

Description

[philippe (part-time)]

STR:
1. load http://www.mozilla.com/en-US/
2. right click somewhere on the big background image

ER: context menu item 'View Background Image' is active
AR: 'View Background Image' is greyed out.

Additionally: the background images don't show up in the media tab of the page info window

Works
http://hg.mozilla.org/mozilla-central/rev/d9d4bf676f65
Gecko/20090219 Minefield/3.2a1pre

Fails
http://hg.mozilla.org/mozilla-central/rev/35d78a340566
Gecko/20090220 Minefield/3.2a1pre

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d9d4bf676f65&tochange=35d78a340566

Possibly bug 322475 ? (in which case, this probably should be a core bug)

Comment 2

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

This should not be marked as a duplicate of a bug predating the regression.

Comment 3

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

This is probably because the getComputedURL function doesn't handle background-image's computed value being a list, as it now is.  (And I'd strongly prefer avoiding CSSPrimitiveValue use anyway.)

Comment 4

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

(In reply to comment #3)
> (And I'd strongly prefer avoiding CSSPrimitiveValue use anyway.)

On further consideration, I think it's probably better than the alternatives.

Comment 5

[Philip Chee]

> This is probably because the getComputedURL function doesn't handle
> background-image's computed value being a list, as it now is.

It's failing here as well |this.target instanceof HTMLHtmlElement|

Comment 6

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

(In reply to comment #5)
> > This is probably because the getComputedURL function doesn't handle
> > background-image's computed value being a list, as it now is.
> 
> It's failing here as well |this.target instanceof HTMLHtmlElement|

If you're seeing it failing there, then either you're debugging it wrong or you're seeing a different problem.

I can tell you pretty much for sure that the multiple backgrounds patch would have broken this for the reason in comment 3.

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch

This just takes the first background image, which is bad when sites start to use multiple background images extensively, but should do for now.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 379606 [details] [diff] [review]
patch

This is actually pretty bad. I cannot think of any good reason to pick the first background and not the second.

It's probably the right time to move this feature to the page info dialog. Ask ux.

Comment 9

[Markus Stange [:mstange]]

Until the right UI is worked out, a menu item that shows the right image in 99% of all cases is preferable to a menu item that's always greyed out, I think.

Comment 11

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

(In reply to comment #9)
> Until the right UI is worked out, a menu item that shows the right image in 99%
> of all cases is preferable to a menu item that's always greyed out, I think.

I agree; multiple backgrounds are still going to rare on the Web for quite a while, since IE doesn't support them.

Comment 12

[megsuma]

Actually, in my report of this bug, marked as a dupe, I was reporting this as an issue of a single background.  The image is a png from the body section of a linked-in stylesheet. The bug I submitted was https://bugzilla.mozilla.org/show_bug.cgi?id=509658

Comment 13

[megsuma]

Another comment from me :D I'm running the latest build of Minefield/Namoroka whathaveyou - 3.6a2pre.  If you browse to http://newsofthebrutal.org/ and right-click, you should see the grayed out option for "View Background Image."

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

We should either fix this, or remove the menuItem. Having it always greyed out is helping no one.

David: does this belong here or in another component?

Comment 15

[Justin Dolske [:Dolske]]

We have context menu tests, but I guess this particular one wasn't handled. Would be good to add a test for View Background Image being both present and enabled.

Comment 16

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

Seems to me like it's in the right component.

Comment 17

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

Oh, and it looks like dolske reset beltzner's setting the blocking flag to +; let's see if I have the permissions to fix that...

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

UX suggestions:

- when there's only one background image, we should just show the item as usual
- when there are multiple background images, we should not show the menuItem
- as a followup, we should have a new menuItem "View Background Images" which open the multiple images in tabs

Comment 19

[IU]

(In reply to comment #18)
> - when there are multiple background images, we should not show the menuItem
> - as a followup, we should have a new menuItem "View Background Images" which
> open the multiple images in tabs

Instead, how about "View Background Image" on the background image over which right-clicking occurred?

Comment 20

[Dão Gottwald [:dao]]

Attachment: patch

Comment 21

[megsuma]

(In reply to comment #19)
That might work, but what about overlapping backgrounds with transparency? I think opening multiple in tabs would be a bit better, or perhaps a submenu:
View Background Images... -> bg1.jpg, bg2.jpg, ... and leave the tabbing up to the user - middle-click or ctrl+click an image from the list to open in a new tab or on the whole "View Background Images..." to open all in tabs.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Everyone please remember we're talking about something that isn't widely supported on the web, so we're into corner cases and assumptions about how things will work. The more important aspect of this bug, IMO, is restoring the function for the 90+% of cases out there where there's a single background image. Follow-ups can be filed on changing the behaviour of the multiple background case if that comes up a lot in the future.

Comment 23

[Asaf Romano (gone)]

Comment on attachment 399569 [details] [diff] [review]
patch

For Now.


r=mano.

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/7bfe69ba4bc9

Comment 25

[[not reading bugmail]]

Tested trunk on www.mozillazine.org with multiple backgrounds in the header; Top level images are selected before the lower background image and view background image stays greyed out when not right clicking on any image.  

So it works as expected for UI's suggested fix in comment 19.

Nice work guys.

Comment 26

[[not reading bugmail]]

It also works for case in comment 18 for a single background image on the page and case of comment 21 with two background images where the top level background image looks transparent on www.mozillazine.org.

Though I found a case it doesn't do anything and maybe just an edge case bug in itself.  Where if you right click on a bug page such as this, it has 2 image labeled as background, but I cannot right click on Mozilla image above 
and see the generic image context menu items, nor do I see the view-background image context menu item, even though it appears to be on top of the main page background image and labeled as a background type in page info.  Maybe its actually below it?

Comment 27

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/303d5d3380b4