50514 - div with background-color has "copy image location" in context menu

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P3)

Description

[Jesse Ruderman]

<div style="background-color:#dfd;">
  Text<br>
  Text
 </div>

when i right-click in the div (but not on the text), "copy image location" 
shows up in the context menu.  when i click on it i get "rgb(221,255,221)" in 
the clipboard.  

this seems intentional because i seem to get the correct color.  i think the 
context menu item should say "copy background color" and not "copy image 
location" in this case.  it also seems a bit arbitrary that it would work with 
css background colors and not body background colors, or any pixel the user 
right-clicks on.

Comment 1

[Jesse Ruderman]

Attachment: test case

Comment 2

[Mike Pinkerton (not reading bugmail)]

nothing to do with toolkit.

Comment 3

[andreas]

Another Testcase can be seen at http://thienemann.net.
Right-clicking on the background (outside the table) shows again mozilla
interpreting it as an image "rgb(153,153,153)".

Right-clicking onto the table in the middle however shows the browser not
interpreting the background as n image.

The problem with the browser interpreting a "empty" background as an image is
the disapearance of the "view source" and "view page info" commands.

Comment 4

[Bill Law]

Weird; I think this is a side-effect of the way we implement such background
colors.  I'll have to look deeper.  The code that handles this is at
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/nsContextMenu.js#311
which is only looking for background-image and background style attributes, not
background-color.

Comment 5

[David Hyatt]

Attachment: Patch demonstrating how the context menu code should be using getComputedStyle

Comment 6

[David Hyatt]

Instead of using .style, we should be using getComputedStyle.

Comment 7

[Bill Law]

->mozilla0.9.7

Comment 8

[Bill Law]

Mass moving bugs that won't get fixed this milestone.

Comment 9

[Bill Law]

Attachment: Patch to use getComputedStyle

This is a little more complicated because getComputedStyle("background-image")
returns "none" rather than null (why is that?).

So I have to loop, test each attr for null||"none", etc.

Comment 10

[Stephen P. Morse]

Comment on attachment 65555 [details] [diff] [review]
Patch to use getComputedStyle

r=morse

Comment 11

[Boris Zbarsky [:bzbarsky]]

> returns "none" rather than null (why is that?)

Because the default value in the CSS spec is "none".  So the computed value is
"none" unless some value is set.  "" would not be a valid background-color value
at all.

By the way, the computed value of "background" and "list-style" will always be
"".  So there is no reason to check them.  If there is a background image it'll
be listed in the background-image style, always, no matter how it was set.  Same
goes for list-style-image.

As far as that goes, the fact that list-style-image returns "" instead of "none"
is wrong.  I just filed bug 120719 on that.

Comment 12

[Bill Law]

Thanks for the explanation.  Should I recode this patch to use the simpler
technique that was in place previously (minus the checks for "background" and
"list-style", then?  That would be blocked by 120719, though...

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm not sure the simpler thing will work... You probably just want to remove
"list-style" and "background" from the list.

Tou could change |if ( attr && attr != "none" )| to |if (attr != "none")|, but
then this _would_ be blocked by bug 120719.  (that's a very simple change, has
r=, and I should be able to land it once tree reopens for 0.9.9, but....)

Or you could do:

var cssAttr = propList.getPropertyValue("background-image");
if (cssAttr == "none") {
  cssAttr = propList.getPropertyValue("list-style-image");
}
if (cssAttr != "none") {
 // get the url and all that
}

though this would again depend on bug 120719.

Comment 14

[Bill Law]

Attachment: Revised patch more in keeping with the current coding style.

That's what I had in mind.  It's a little different to account for not being
able to initialize csAttr with a simple logical expression, but keeps the style
of the old code.

When the other bug is fixed, we can remove the "&& csAttr" (but won't have to).


Reviewer's choice.

Comment 15

[Boris Zbarsky [:bzbarsky]]

bug 120719 has r/sr, so it's landing as soon as the tree reopens (it does not
seem critical for 0.9.8).

Comment 16

[Bill Law]

So which version of the fix do you like better?  I'll bug hyatt for a sr.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I like the later version, but you can get rid of the "&& cssAttr" part -- bug
120719 has been fixed.   With that, r=bzbarsky

Comment 18

[Bill Law]

Attachment: Final draft

I removed the "&& csAttr".  Looking for sr= from hyatt.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 67184 [details] [diff] [review]
Final draft

r=bzbarsky

Comment 20

[Boris Zbarsky [:bzbarsky]]

Was just talking to blake about this stuff.  We don't want to set this.onImage
for background images.  We want to set this.hasBGImage:

        this.showItem( "context-viewbgimage", this.hasBGImage && !this.onImage );

Comment 21

[Bill Law]

Attachment: Forget that previous crap.

Well, it's amazing what a little testing will do.

I deleted the code we were trying to fix with the prior patches.  It's totally
bogus.

First, list-image-style can't be looked for because it doesn't make any sense. 
the problem is that the image specified via that attribute doesn't appear
anywhere near where that style attribute appears.  You can give that style to a
non-list and it still applies.	If you apply it to a <ul>, for example, then
the entire <ul> has that style attribute and the images only appear off to the
left of the <li>s.  The user will click on anything and get these mysterious
image-related context menu items.

As for background-image, it seems to be plain broken.  The getPropertyValue
*always* returns "none."  That's probably a bug, but not this one.

However, I did find other code that looked for background images and it didn't
work worth a crap either.  I fixed that code with this latest patch.  This gets
"view background image" options to appear on table/th/td elements with
background attributes.

Boris, would you mind having another look at this.  Sorry.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Why |localname.search( /TD|TH|TABLE/ )| ?  What about BODY?

Comment 23

[Bill Law]

That's covered by code higher up that looks explicitly at the <body> element. 
This code was only handling the case where we're bubbling up the content hierarchy.

But now that you mention it, I think maybe this is wrong.  That <body> code
should be removed and we should look for BODY here.  Otherwise, a page with a
<body> background will always want you to view that and you won't ever be able
to view a background in a table on such a page.

I'll test this out later today and see if that alternative works better.  Good
catch.

Comment 24

[Bill Law]

Attachment: Last one, I promise!

OK, this one works, all the time (as best I can determine).

There's may be another bug lurking: if you specify background="image-url" for a
<table>, then tableNode.background is not set (it is for <th>, <td>, and
<body>).  That's why the code here was previously written to use
elem.background instead of elem.getAttribute.  I think using getAttribute is
OK, so I'm using that and it can probably stay that way even if the quirk with
<table> .background properties is fixed (I don't even know if it's a bug).

The bottom line is that this patch fixes this bug and gets background image
viewing working in all cases.

It also cleans up the code some.  Please forgive the circuituous route I took
to get here.

Comment 25

[David Hyatt]

sr=hyatt

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 68630 [details] [diff] [review]
Last one, I promise!

>-            } else if ( this.target.localName.toUpperCase() == "HTML" ) {

We can't remove this case.... This is to catch a situation where the page
is short so BODY is not at tall as HTML.  The user right-clicks on HTML
and does not get the "view background image" item because bubbling out
from HTML will not catch BODY...

>+                     localname.search( /TD|TH|TABLE|BODY/ ) != -1 &&

Make that /^(?:TD|TH|TABLE|BODY)$/ please.  That makes it not match things
like "THAG".  In the future we want to allow any elements in there, I think,
but I want to do some computed style perf work first.  :)

r=bzbarsky if you leave the check for HTML in and fix the regexp.

Comment 27

[Bill Law]

Attachment: So I lied.

Incorporates changes suggested by Boris.

I didn't think handling <html> was necessary (couldn't see how it could have
anyc ontent that wasn't contained by the <body> tag) but I could reproduce the
problem if I clicked really close to the edge of the background.  Adding that
check back in fixed that.

I modified the regexp as suggested.  I was leery of checking background= for
anything other than table/tr/td/body.  If some page puts background= on a <p>,
I'm not sure the user should be able to "view background image."  On the other
hand, it could make for a neat easter egg kind of effect ("go to www.foobar.com
and right click on Britney's name, then choose view-background-image to see
something really kewl!"; best viewed using Mozilla, of course :-).

That said, I think the computed-style check should not be constrained to only
those elements.  I'm going to work on relaxing that restriction; I'll post
another patch when and if I get that working.  I'll have to enhance my
testcase, first.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 68975 [details] [diff] [review]
So I lied.

The simplest way to click outside of <body> is to have a short page.  <body> is
only as tall as the content, <html> is always at least the height of the
vieport

This looks good, modulo comments about checking style on all elements.

Oh, and also, could we anchor the "url()" regexp to the string's beginning?

Comment 29

[Bill Law]

Attachment: How about this one, then

I tweaked the code to handle background-image styles on arbitrary document
elements.

Boris, I inserted a "^" at the beginning of that other regexp, but I'm not
100%confident about that being all that's needed.

I've found at least 3 bugs:

a. Given a page with a background-image style on the <body>, a <table> with a
background-image style, and a <td> with a background-image, and a <p> with a
background-image (all these nested), then doing "view background image" when
right-clicking on text in the <p> views the <td>'s background image.  This
works ok when clicking on empty space in the same <p>.
b. Same page (<body> with background-image), <table> with bgcolor, <td> with no
background-image, <p> with background-image, then right-clicking on text
produces no "view background image" option in menu.  Again, clicking in blank
area in same <p> works OK.
c. Same <body>, <table>, and <td> as in b., but this time the <p> has no
background-image style.  Right click has no "view background image" option (on
either text or empty space in the <p>).  This works OK iff I specify the
<body>'s background image via background=.

It seems as if some constructs are fouling up the getting of the computed
background style (specifying an explicit background= attribute seems to always
"fix" things).

There were not as many problems with my previous patch.  That's because that
patch computed the background-image style at each level as it bubbled up the
content hierarchy.  I removed that code because I thought it was redundant
since any computed style in effect for the parent would also be in affect for
the child nodes (unless overridden).  But maybe I'm confused on that.

Anyway, to make this work properly, in context of the (apparent) current bugs
in the computed style lookup, I'll have to add back in the code that gets the
computed style as we bubble up (and remove the this.target specific code).

Man, I'm getting tired of this one...

Comment 30

[Boris Zbarsky [:bzbarsky]]

> since any computed style in effect for the parent would also be in affect for
> the child nodes

Actually, it would not....  The background properties are not inherited unless
you specifically say they should be in your stylesheet (unlike, say, the color
properties).  They default to transparent.  I think that covers all three
problems you're seeing...

Thanks for your patience on this one, Bill.

Comment 31

[Bill Law]

OK; I'm by no means a CSS expert.  But it seems like if a style attribute 
applies (i.e, I can see the background image behind an element), then the 
computed style should reflect that.  But that's only common sense, maybe.

So it sounds like I should move the background-image check into the bubbling-up 
portion of this routine, and then all will be well.  The only issue is 
performance.  Is that something to be worried about?

Comment 32

[Bill Law]

Attachment: patch v23.9

OK, this one seems to work for all the tests I could throw at it.

I embellished the code that looked for <body> background when <html> was
clicked on to also handle cases where that background was specified via CSS.

I removed the replace() method call to strip url() because that's done already
by getPropertyValue().

The only thing I'm not 100% happy with (aside from the bugs Boris finds in my
code :-) is this:  when there's a background image "obscured" by something else
(e.g., a <td> with bgcolor= inside a <body> with background=, then you can
"view background image" even though, technically, there's no image where you
clicked.

That can be "fixed," of course; I'm just not sure it should be.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> The only issue is performance.  Is that something to be worried about?

I'm not sure, to be truthful...  That code is decently fast at the moment and
I'll try to make sure it stays that way....

> I removed the replace() method call to strip url() because that's done already
> by getPropertyValue().

Oops.  That would be a bug in getPropertyValue(). Please add that back in and
I'll fix getPropertyValue() to return valid css....

r=bzbarsky with that one change.

Comment 34

[Bill Law]

Attachment: Patch v3.14159265

Check out this one.  It should handle url(whatever) when getPropertyValue is
fixed.

I'm taking hyatt's and bzbarsky's sr=/r= as still applying.  No patch has been
tested, reviewed, modified, retested, and re-reviewed as much as this one.

Whew.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 69287 [details] [diff] [review]
Patch v3.14159265 

r=bzbarsky still applies, yes.
Bill, thanks for spending the time on it.  :)

Comment 36

[Peter Trudelle]

nsbeta1- per Nav triage team

Comment 37

[sairuh (rarely reading bugmail)]

taking qa contact, unless jrgm minds ;)

found this while testing bug 92749. i've at test at
http://hopey.mcom.com/tests/background-img-css.html (sorry, internal server)
where i specify a background image for <body> via CSS. currently the context
menu won't display "view background image". correct me if i'm wrong, but this
would fix this case, yes?

Comment 38

[Boris Zbarsky [:bzbarsky]]

You are not wrong.  This patch should fix that.

Comment 39

[Bill Law]

too late with that nsbeta1-; fixed

Comment 40

[sairuh (rarely reading bugmail)]

tested using 2002.04.16 comm branch bits on linux rh7.2, mac 10.1.3 and win2k.

using jesse's test case (attachment 13604 [details]), no longer see "copy image location"
in the context menu. using my test case (url in comment 37), "view background
image" in the context menu works fine.