528426 - Add "View Image Info" for background images

Status: NEW

(Firefox :: Page Info Window, enhancement)

Description

[Tanner M. Young [:tmyoung]]

I am wondering if we should add the "View Image Info" option to background images.  With the frequency that images are used as background elements, it's often hard to tell when something is an img tag or background element.

Should we add this?  If we do, do we want it the menu item to say "View Image Info" or "View Background Image Info"?  I assume we want the former to make thing simpler, and it would allow us to still add it for 1.9.2, if we want.

Comment 1

[Tanner M. Young [:tmyoung]]

Particularly after Bug 539068, I think this certainly falls into the Wontfix category.

Comment 2

[Tanner M. Young [:tmyoung]]

(In reply to comment #0)
> I am wondering if we should add the "View Image Info" option to background
> images.  With the frequency that images are used as background elements, it's
> often hard to tell when something is an img tag or background element.
> 
> Should we add this?  If we do, do we want it the menu item to say "View Image
> Info" or "View Background Image Info"?  I assume we want the former to make
> thing simpler, and it would allow us to still add it for 1.9.2, if we want.

Whatever we do, let's not do View Background Image Info...

I think this is more useful now that Bug 544710 is in the works.  If we used the same context menu item for images and backgrounds, all we would need to worry about is in cases like Bug 539068 to give the image priority over its background.  If we did that, this would be fairly simple.

I'll put something together to see how feasible this is.

Comment 3

[Tanner M. Young [:tmyoung]]

Attachment: WIP No. 1

This is a really, really basic patch.  It works for all elements except the body element, and it regresses Bug 539068.  My plan is to fix these two problems (I'll look more into the first and quickly take care of the second) and then request review from Dao.

Comment 4

[Tanner M. Young [:tmyoung]]

Attachment: WIP No. 2

I have "roughly" fixed the regression of Bug 539068, but I don't know why it doesn't work with the body element.  Perhaps there is another element between the two that needs to be accounted for.  I also added a shouldShow check for bgImages: do we want that?  We have it for the current View Background Image feature.

Comment 5

[Tanner M. Young [:tmyoung]]

Attachment: WIP No. 3

I still have not made any progress on why body tag backgrounds fail to work, the image is in the list, but the element that is passed to Page Info is not gImageElement, and there is no tag on top of the body tag.  I have moved View Image Info lower in the context menu to where View Background Image is, which doesn't affect the menu for normal images but prevents View Image Info from being at the top of the context menu on backgrounds.

Comment 6

[Tanner M. Young [:tmyoung]]

Attachment: WIP No. 4

Groomed some things but still no idea why the body element refuses to work.

Comment 7

[Tanner M. Young [:tmyoung]]

Attachment: WIP No. 5

This fixes the body tag problem.  What this patch does, rather looking through parent elements for a background, it just looks at the current element.  I've found that the vast majority of sites that treat background-image as <img> do so with standalone elements on the surface of the view area.  This only detects those and ignores body elements and other things that are not at the surface.  This would keep the web developers happy, reduce the confusion of the element and remove it for most, if not all, of the useless places that View Background Image appears.

Comment 8

[Dão Gottwald [:dao]]

Well, a right click left to the textarea I'm typing in gives me the body element which apparently has a decorative background image...

Comment 9

[Tanner M. Young [:tmyoung]]

Attachment: Patch ready for UI review

True, it does show it, but only for the areas where the body element is not covered by another tag.  That is intentional for what this patch is trying to achieve at the moment.  Arguably, there are <img>s that are purely decorative, but we don't differentiate between those.  Notwithstanding that, this dramatically reduces the number of confusing places that the feature appears, which in turn reduces clutter.

The patch so far:
1. View Image Info appears only on tags that have a background image, if there is a parent tag with a background image, it is ignored.
2. Fix for Bug 286028 and Bug 287048
3. Support for the body tag backgrounds, when needed
4. Move View Image Info where View Background Image currently is (wont affect the image context menu)
5. Prevent Bug 539068 from reappearing

Comment 10

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

Why don't we just treat background images like images? So if the user is right-clicking on a background image, they should see the same items as if it were a foreground image, and they should work the same. 

That it's a background image is really an implementation detail of the HTML or CSS.

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> That it's a background image is really an implementation detail of the HTML or
> CSS.

It is, but not an irrelevant one. Separation of content and styling is well-established in web design. Background images are often used to create textures (which is also their intended use), foreground images aren't (again by design).

Comment 12

[Tanner M. Young [:tmyoung]]

(In reply to comment #11)
> It is, but not an irrelevant one. Separation of content and styling is
> well-established in web design. Background images are often used to create
> textures (which is also their intended use), foreground images aren't (again 
> by design).

The difficulty is policing that.  Google is one example: half of the time for their logo they use a background sprite and the other half of the time they use an <img> tag--depending on results of their performance testing.  There is no visible difference between the two methods and it can be confusing as to why the context menu is different for some of the images.  Wikimedia sites do a similar thing; for their logos they use

<a title="Visit the main page" href="/wiki/Main_Page" style="background-image: url('http://upload.wikimedia.org/wikipedia/en/b/bc/Wiki.png');"></a>

instead of

<a title="Visit the main page" href="/wiki/Main_Page"><img src="http://upload.wikimedia.org/wikipedia/en/b/bc/Wiki.png" /></a>

Both use cases are equally standards compliant.  There is no enforceable standard for preventing developers from using background images for non-decorative purposes, and there are situations where this approach can lead to performance gains in some browsers.

Comment 13

[Dão Gottwald [:dao]]

I'm not saying that people don't use background-image that way. Often, however, it's not used like that, and there image related items would clutter the context menu.

Comment 14

[Tanner M. Young [:tmyoung]]

Attachment: Combination patch for this bug and Bug 544710

This patch combines the two bugs in question (this and Bug 544710), if we want both changes applied.  It removes the unneeded functions and logic and preserves the little functionality needed for the special case of top-level backgrounds with View Image Info.  I haven't addressed the automated testcases for context menus, and I will wait until the concept is approved before I address that or any coding issues with the patch.

Beltzner: This way, if you approve both patches, we don't have to merge them later.

Comment 15

[Tanner M. Young [:tmyoung]]

I don't have time to actively work on this or apply any changes to this patch, should ui-review or others reviews require it.

Comment 16

[Tanner M. Young [:tmyoung]]

I might be able to still work on this later, but certainly not for the next week or so; however, it did occur to me, we have just added ogg audio and video to Page Info.  If we added moz-gradient and other media formats, then we would have a complete listing of media, making the feature powerful and different from things like Firebug.  This would resolve Dao's concern that a jpg gradient is listed while a moz-gradient isn't.  In addition, this would make this tool into "View Media Info" instead of limiting it to "View Image Info," which would allow the feature to provide unique and useful information about any media on the page instead of just being a fancy reimplementation of the obsoleted "Properties" context menu item.

Comment 17

[Dão Gottwald [:dao]]

-moz-*-gradient is not a medium, it doesn't communicate anything...

Comment 18

[Tanner M. Young [:tmyoung]]

I have time to work on this again.

Comment 19

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

Comment on attachment 429887 [details] [diff] [review]
Patch ready for UI review

Not sure if this is still relevant, am sure I'm not the right reviewer :)