Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in seamonkey2.1a1

Status

SeaMonkey
General
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

({regression})

Trunk
seamonkey2.1a1
regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

8 years ago
<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>
(Assignee)

Comment 1

8 years ago
Created attachment 429725 [details] [diff] [review]
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.
Attachment #429725 - Flags: review?(neil)

Comment 2

8 years ago
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

8 years ago
(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?
(Assignee)

Comment 4

8 years ago
> 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.
(Assignee)

Comment 5

8 years ago
Created attachment 430055 [details] [diff] [review]
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.
Attachment #429725 - Attachment is obsolete: true
Attachment #430055 - Flags: superreview?(neil)
Attachment #430055 - Flags: review?(neil)
Attachment #429725 - Flags: review?(neil)

Comment 6

8 years ago
Created attachment 430075 [details]
Flowers pattern for multibackground transparency demo

Comment 7

8 years ago
Created attachment 430078 [details]
Multibackground transparency demo
(Assignee)

Comment 8

8 years ago
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)
(Assignee)

Comment 9

8 years ago
Created attachment 430090 [details] [diff] [review]
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.)
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 ;-)

Updated

8 years ago
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? ;-)
(Assignee)

Comment 12

8 years ago
Created attachment 430599 [details] [diff] [review]
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.
Attachment #430090 - Attachment is obsolete: true
Attachment #430599 - Flags: superreview?(neil)
Attachment #430599 - Flags: review+

Updated

8 years ago
Attachment #430599 - Flags: superreview?(neil) → superreview+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 13

8 years ago
Pushed as http://hg.mozilla.org/comm-central/rev/db414705ccf9
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.