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)
This should not be marked as a duplicate of a bug predating the regression.
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.)
(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.
> 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|
(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.
Created attachment 379606 [details] [diff] [review] 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 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.
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.
(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.
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
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."
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?
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.
Seems to me like it's in the right component.
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...
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
(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?
Created attachment 399569 [details] [diff] [review] patch
(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.
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 on attachment 399569 [details] [diff] [review] patch For Now. r=mano.
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.
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?