979292 - Rule view has image pop-ups on appear on container

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Joe Walker [:jwalker]]

Could happen when a background-image contains both a color and a URL

Comment 1

[Patrick Brosset <:pbro>]

STR:

- Open the inspector on this page
- Select the body node
- Find the `background-image: url('noise.png'), linear-gradient(#D7D3C8, #F6F4EC 400px);` declaration in the rule-view
- Hover over the `noise.png` url
==> The image preview tooltip is correctly positioned
- Click on one of the color swatches in the same declaration
- Change the color and hit ENTER
- Hover over the `noise.png` url again
==> The image preview is wrongly positioned.

Comment 3

[Patrick Brosset <:pbro>]

I think this is related to something I've wanted to fix for a long time: our rule-view panel refreshes elements more than it should. So whenever you change a value, I think rule objects get re-created/re-applied and therefore, the element that served as an anchor to the image preview tooltip before doesn't exist anymore.
When using the color picker, every new pixel the color dragger is moved to causes a new color value to be set in the property, and therefore, I'm guessing the rule object gets re-created too in this case.

Having said this, when hovering over a URL in the rule-view, we get a mouseover event, with an associated target, so I don't quite know yet why the XUL panel anchor is wrong.

I'll look into this now.

Comment 4

[Patrick Brosset <:pbro>]

I've noticed another bug, and looking at the code, I think both are related:
In the rule-view, change a color with a colorpicker, then hover over an image => the color is back to its previous value.
There's some clean-up to be done in the rule-view / tooltip mechanism.

Comment 5

[Alice0775 White]

This problem does not happens in Light Theme(it is necessary to restart browser after switching the theme)

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Tracking here. We won't probably block the release for that but since the inspector is a great feature of FF, it would be nice to have it fixed.

Comment 7

[Patrick Brosset <:pbro>]

I'm also working on bug 966424 in parallel, and it seems that what I fixed for that bug also fixed issues for this bug.
Indeed, that bug is about intermittent tooltip test failures, so in order to fix it, I'm working on making the whole show/hide tooltip process more solid, and that helped fix this issue too.
So let's make a dependency here.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug979292-colorpicker-and-image-tooltips-in-ruleview v1.patch

Some clean-up of the tooltip's display mechanism was done in bug 966424, this helped get rid of the issue here.

Here is a list of things done:
- The swatch-based tooltip now reset the `activeSwatch` object once hidden. This fixes a bunch of problems related to pressing ESC after the colorpicker was hidden, or hovering over an image uri with the colorpicker shown.
- The rule-view's preview function that is called when dragging the cursor on the colorpicker now really only previews the change. It used to re-generate the whole rule-view content, leading the swatch DOMNode being re-created, and therefore to bugs like bug 980225 (which I'll check and close as dup after this lands if really fixed).
- I've split the big colorpicker test in multiple smaller tests (it was timing out, see bug 950041) and added a bunch of new tests to cover the usecases of this bug.

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=86ea33f5fab0

Comment 9

[Heather Arthur [:harth]]

Comment on attachment 8399923 [details] [diff] [review]
bug979292-colorpicker-and-image-tooltips-in-ruleview v1.patch

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

looking good.

::: browser/devtools/styleinspector/rule-view.js
@@ +664,5 @@
>    /**
> +   * Just sets the value and priority of a property, in order to preview its
> +   * effect on the content document.
> +   */
> +  previewPropertyValue: function(aProperty, aValue, aPriority) {

The rest of the funcs seem to have @param comments so I'd add them here too.

@@ +1124,5 @@
> +   * Which type of hover-tooltip should be shown for the given element?
> +   * This depends on the element: does it contain an image URL, a CSS transform,
> +   * a font-family, ...
> +   * @param {DOMNode} el The element to test
> +   * @param {String} The type of hover-tooltip

@return not @param

::: browser/devtools/styleinspector/test/browser.ini
@@ +75,5 @@
> +[browser_ruleview_colorpicker_04.js]
> +[browser_ruleview_colorpicker_05.js]
> +[browser_ruleview_colorpicker_06.js]
> +[browser_ruleview_colorpicker_07.js]
> +[browser_ruleview_colorpicker_08.js]

It would be handy to have the names reflect what the tests are testing. If that's hard to describe then no big deal though.

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Heather Arthur [:harth] from comment #9)
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +664,5 @@
> >    /**
> > +   * Just sets the value and priority of a property, in order to preview its
> > +   * effect on the content document.
> > +   */
> > +  previewPropertyValue: function(aProperty, aValue, aPriority) {
> 
> The rest of the funcs seem to have @param comments so I'd add them here too.
You're right. I have added comments to this function and a few others that were missing them.

> @@ +1124,5 @@
> > +   * Which type of hover-tooltip should be shown for the given element?
> > +   * This depends on the element: does it contain an image URL, a CSS transform,
> > +   * a font-family, ...
> > +   * @param {DOMNode} el The element to test
> > +   * @param {String} The type of hover-tooltip
> 
> @return not @param
Thanks. Done.

> ::: browser/devtools/styleinspector/test/browser.ini
> @@ +75,5 @@
> > +[browser_ruleview_colorpicker_04.js]
> > +[browser_ruleview_colorpicker_05.js]
> > +[browser_ruleview_colorpicker_06.js]
> > +[browser_ruleview_colorpicker_07.js]
> > +[browser_ruleview_colorpicker_08.js]
> 
> It would be handy to have the names reflect what the tests are testing. If
> that's hard to describe then no big deal though.
You're right, it helps naming files better. Done.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: bug979292-colorpicker-and-image-tooltips-in-ruleview v2.patch

This v2 patch addresses Heather's review comments only.

Comment 12

[Patrick Brosset <:pbro>]

Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/24f5a463b606

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/24f5a463b606

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Getting this on Aurora would make porting the styleinspector test rewrite way easier. Any reason we can't do that? :)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

(applies cleanly, fwiw)

Comment 16

[Patrick Brosset <:pbro>]

No reason we can't do that. Let's go for it.

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8401133 [details] [diff] [review]
bug979292-colorpicker-and-image-tooltips-in-ruleview v2.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
This bug was a follow-up of the initial tooltip implementation bugs (765105, 889638).

User impact if declined: 
The image and color-picker tooltips in the devtools inspector may behave weirdly when using one after the other (incorrect position, or colors being reverted).

Testing completed (on m-c, etc.): 
mochitest-browser tests, and on m-c for 2 weeks.

Risk to taking this patch (and alternatives if risky): 
The risk is very minimal considering it's been on nightly for 2 weeks and hasn't generated regressions or more bugs.
The patch touches code in the devtools tooltips, so if there is a risk, it will only have consequences when hovering over image urls in the devtools inspector, or when using the color picker.

String or IDL/UUID changes made by this patch: None

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/becbae5947c9

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

We're too late to take this to Beta, wontfixing there.