Last Comment Bug 549612 - 'View Background Image' context-menu item is always greyed out (Port Bug 482941)
: 'View Background Image' context-menu item is always greyed out (Port Bug 482941)
Status: RESOLVED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Philip Chee
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-02 06:24 PST by Philip Chee
Modified: 2010-03-12 08:08 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 Use the first background image. (1.80 KB, patch)
2010-03-02 06:41 PST, Philip Chee
no flags Details | Diff | Splinter Review
Patch v1.1 Fix review comments. (1.81 KB, patch)
2010-03-03 08:15 PST, Philip Chee
no flags Details | Diff | Splinter Review
Flowers pattern for multibackground transparency demo (39.15 KB, image/jpeg)
2010-03-03 09:33 PST, neil@parkwaycc.co.uk
no flags Details
Multibackground transparency demo (429 bytes, text/html)
2010-03-03 09:35 PST, neil@parkwaycc.co.uk
no flags Details
Patch v1.2 Handle multibackground transparency test case. (1.88 KB, patch)
2010-03-03 10:25 PST, Philip Chee
neil: review+
Details | Diff | Splinter Review
Patch v1.3 Fix final nits. r=Neil (1.88 KB, patch)
2010-03-05 05:23 PST, Philip Chee
philip.chee: review+
neil: superreview+
Details | Diff | Splinter Review

Description Philip Chee 2010-03-02 06:24:34 PST
<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 2010-03-02 06:41:36 PST
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.
Comment 2 neil@parkwaycc.co.uk 2010-03-02 09:05:56 PST
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 neil@parkwaycc.co.uk 2010-03-02 09:06:46 PST
(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 2010-03-02 09:35:31 PST
> 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 2010-03-03 08:15:08 PST
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.
Comment 6 neil@parkwaycc.co.uk 2010-03-03 09:33:39 PST
Created attachment 430075 [details]
Flowers pattern for multibackground transparency demo
Comment 7 neil@parkwaycc.co.uk 2010-03-03 09:35:20 PST
Created attachment 430078 [details]
Multibackground transparency demo
Comment 8 Philip Chee 2010-03-03 09:46:22 PST
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 2010-03-03 10:25:59 PST
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.)
Comment 10 neil@parkwaycc.co.uk 2010-03-03 12:02:19 PST
(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 neil@parkwaycc.co.uk 2010-03-03 12:05:29 PST
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 2010-03-05 05:23:46 PST
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.
Comment 13 Robert Kaiser 2010-03-12 08:08:51 PST
Pushed as http://hg.mozilla.org/comm-central/rev/db414705ccf9

Note You need to log in before you can comment on or make changes to this bug.