549612 - 'View Background Image' context-menu item is always greyed out (Port Bug 482941)

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

<blockquote>
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
</blockquote>

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Use the first background image.

Bug 482941 decided in the end to not to show the menu item if there were multiple background images. I've decided to port the first proposal, to just pick the first image when there are multiple background images.

Comment 2

[[e-mail]]

Comment on attachment 429725 [details] [diff] [review]
Patch v1.0 Use the first background image.

>-    getComputedURL: function( elem, prop ) {
>+    getComputedURL: function(aElem, aProp) {
Hmm, well, the style of this file is wrong, but that's a separate bug.

>+      var url = aElem.ownerDocument
>+                    .defaultView.getComputedStyle(aElem, "")
>+                    .getPropertyCSSValue(aProp);
Nit: put the .defaultView on the previous line, and line the .s up properly.
(Your fault for renaming the variables.)

>+      if (url instanceof CSSValueList)
>+        url = url[0];
>+      return url.primitiveType == CSSPrimitiveValue.CSS_URI ?
>+             url.getStringValue() : null;
It would be nice if we could extract the image from the Transparency demo:
http://hacks.mozilla.org/2009/11/css-gradients-firefox-36/
(Note that that's not a live demo.)

Comment 3

[[e-mail]]

(In reply to comment #1)
> Bug 482941 decided in the end to not to show the menu item if there were
> multiple background images. I've decided to port the first proposal, to just
> pick the first image when there are multiple background images.
We could count the number of CSS_URI images and require exactly one?

Comment 4

[Philip Chee]

> We could count the number of CSS_URI images and require exactly one?

Err. I meant that Firefox counted the number of background images and if > 1, hid the menu item.

Comment 5

[Philip Chee]

Attachment: Patch v1.1 Fix review comments.

>>-    getComputedURL: function( elem, prop ) {
>>+    getComputedURL: function(aElem, aProp) {
> Hmm, well, the style of this file is wrong, but that's a separate bug.
OK.

>>+      var url = aElem.ownerDocument
>>+                    .defaultView.getComputedStyle(aElem, "")
>>+                    .getPropertyCSSValue(aProp);
> Nit: put the .defaultView on the previous line, and line the .s up properly.
> (Your fault for renaming the variables.)
Fixed.

>>+      if (url instanceof CSSValueList)
>>+        url = url[0];
>>+      return url.primitiveType == CSSPrimitiveValue.CSS_URI ?
>>+             url.getStringValue() : null;
> It would be nice if we could extract the image from the Transparency demo:
> http://hacks.mozilla.org/2009/11/css-gradients-firefox-36/
> (Note that that's not a live demo.)
I would think that the current patch should work.

Comment 6

[[e-mail]]

Attachment: Flowers pattern for multibackground transparency demo

Comment 7

[[e-mail]]

Attachment: Multibackground transparency demo

Comment 8

[Philip Chee]

Comment on attachment 430055 [details] [diff] [review]
Patch v1.1 Fix review comments.

oh, hmm. The css is on the body. Let me think about this.

Comment 9

[Philip Chee]

Attachment: Patch v1.2 Handle multibackground transparency test case.

> It would be nice if we could extract the image from the Transparency demo:
> http://hacks.mozilla.org/2009/11/css-gradients-firefox-36/
> (Note that that's not a live demo.)

Comment 10

[[e-mail]]

(In reply to comment #8)
> The css is on the body.
That was just laziness on my part, rather than using the class ;-)

Comment 11

[[e-mail]]

Comment on attachment 430090 [details] [diff] [review]
Patch v1.2 Handle multibackground transparency test case.

>+      var url = aElem.ownerDocument.defaultView
>+                     .getComputedStyle( aElem, "" )
>+                     .getPropertyCSSValue( aProp );
>+
>+      if ( url instanceof CSSPrimitiveValue )
>+        url = [ url ];
That blank line there looks odd to me. But I could live with a blank line before the for loop.

>+      for ( let i = 0; i < url.length; i++ )
Did you think I'd let you slip that past me? ;-)

Comment 12

[Philip Chee]

Attachment: Patch v1.3 Fix final nits. r=Neil

Carrying forward r=Neil. This patch touches the UI so asking for sr

>>+      var url = aElem.ownerDocument.defaultView
>>+                     .getComputedStyle( aElem, "" )
>>+                     .getPropertyCSSValue( aProp );
>>+
>>+      if ( url instanceof CSSPrimitiveValue )
>>+        url = [ url ];
> That blank line there looks odd to me.
Fixed.

> But I could live with a blank line
> before the for loop.
Fixed.

>>+      for ( let i = 0; i < url.length; i++ )
> Did you think I'd let you slip that past me? ;-)
Fixed.

Comment 13

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/db414705ccf9