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

RESOLVED FIXED in seamonkey2.1a1

Status

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: philip.chee, Assigned: philip.chee)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

<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>
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.
Attachment #429725 - Flags: review?(neil)
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.)
(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?
> 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.
>>-    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.
Attachment #429725 - Attachment is obsolete: true
Attachment #430055 - Flags: superreview?(neil)
Attachment #430055 - Flags: review?(neil)
Attachment #429725 - Flags: review?(neil)
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.
Attachment #430055 - Flags: superreview?(neil)
Attachment #430055 - Flags: review?(neil)
> 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.)
Attachment #430055 - Attachment is obsolete: true
Attachment #430090 - Flags: review?(neil)
(In reply to comment #8)
> The css is on the body.
That was just laziness on my part, rather than using the class ;-)
Attachment #430090 - Flags: review?(neil) → review+
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? ;-)
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.
Attachment #430090 - Attachment is obsolete: true
Attachment #430599 - Flags: superreview?(neil)
Attachment #430599 - Flags: review+
Attachment #430599 - Flags: superreview?(neil) → superreview+
Keywords: checkin-needed
Pushed as http://hg.mozilla.org/comm-central/rev/db414705ccf9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.