Closed Bug 549612 Opened 15 years ago Closed 15 years ago

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

Categories

(SeaMonkey :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a1

People

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

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

<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.
Attached patch Patch v1.1 Fix review comments. (obsolete) — Splinter Review
>>- 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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: