1026921 - Only one background image is shown in popup on style rules that have multiple backgrounds

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Michael Beckwith]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140615004003

Steps to reproduce:

have a DOM element that has multiple backgrounds assigned to it. Hover over both in the Inspector tab and the rules panel on the right.


Actual results:

Only the first background image is ever shown in the displayed popup to show the image in real time. Hovering over second image showed the first image.


Expected results:

Currently hovered image should change and be displayed in the popup.

Comment 1

[Sami Jaktholm]

Attachment: styleinspector-multiple-background-images.patch

The URL for tooltip image is parsed from the property value which always returns the first url() declaration. Here's a patch that uses the href of the hovered link instead.

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8447619 [details] [diff] [review]
styleinspector-multiple-background-images.patch

Review of attachment 8447619 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Looks good to me.
Just a couple more cleanups while we're at it.

::: browser/devtools/styleinspector/rule-view.js
@@ +1235,5 @@
>        type = overlays.VIEW_NODE_IMAGE_URL_TYPE;
>        value = {
>          property: getPropertyNameAndValue(node).name,
>          value: node.parentNode.textContent,
> +        url: node.href,

Indeed, much better since href is already resolved to the absolute URL.

::: browser/devtools/styleinspector/style-inspector-overlays.js
@@ +337,5 @@
>  
>      if (type === TOOLTIP_IMAGE_TYPE) {
>        let dim = Services.prefs.getIntPref(PREF_IMAGE_TOOLTIP_SIZE);
> +      // nodeInfo contains an absolute uri
> +      let uri = nodeInfo.value.url;

Thanks for simplifying this!

Looks like getBackgroundImageUriFromProperty had been introduced only for this and since it's not needed anymore, we might as well use this patch to remove getBackgroundImageUriFromProperty from css-logic.js.

Searching the codebase for occurrences of getBackgroundImageUriFromProperty reveals it being used in style-inspector-overlays.js in the _getTooltipType method, but that can easily be removed too:
let dim = ...
let uri = ...
are both unused in this method and should be removed.

Comment 3

[Sami Jaktholm]

Attachment: styleinspector-multiple-background-images-v2.patch

Thanks for the review. Here's an improved version with getBackgroundImageUriFromProperty completely removed.

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8447873 [details] [diff] [review]
styleinspector-multiple-background-images-v2.patch

Review of attachment 8447873 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect!

Comment 5

[Sami Jaktholm]

Pending try build: https://tbpl.mozilla.org/?tree=Try&rev=fd4147c73e29

Comment 6

[Sami Jaktholm]

Try succeeded except for one unrelated, already known failure (bug 1016310).

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/2a523e4ae004

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2a523e4ae004

Comment 9

[Michael Beckwith]

Confirmed fixed from my end as well in the July 1st nightly build.