1301342 - Class "ruleview-overridden" is gone after opening the color picker from color property of element which contains it

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P1)

Description

[magicp]

Attachment: class-ruleview-overridden-is-gone.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160907030427

Steps to reproduce:

1. Start Nightly
2. Go to about:home
3. Select "#aboutMozilla" element in marketup
4. Select "html" element in Rules view, and check color property has line-through and filter icon.
5. Open the color picker on its color property, then close
6. Step 5 repeat again



Actual results:

In step 5, Class "ruleview-overridden" is gone from color property. In step 6, also filter icon is gone. Please find attached video.

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=1de829f2f1f03e23ca0159bee473d36b9989e62b&tochange=76d556ef9180969ee8d690ed069732faded934a2


Expected results:

Class "ruleview-overridden" should be not gone after opening the color picker from color property of element which contains it.

Comment 1

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1301342.patch

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8794373 [details] [diff] [review]
1301342.patch

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

We need to find a solution that allows us to keep using the xul wrapper here.
Do you have more insights as to why the classname is not updated here? 

(It looks like this problem only occurs on Windows? Haven't been able to reproduce on OSX. Until tomorrow I won't have access to a windows machine so I can't help much with the investigation until then.)

::: devtools/client/inspector/rules/views/text-property-editor.js
@@ +448,5 @@
>      this.ruleView._updatePropertyHighlight(this);
>    },
>    _onStartEditing: function () {
>      this.element.classList.remove("ruleview-overridden");
> +    this.filterProperty.hidden = true;

Not related to this bug ?

::: devtools/client/shared/widgets/Tooltip.js
@@ -483,5 @@
>    // It will also close on <escape> and <enter>
>    this.tooltip = new HTMLTooltip(toolbox, {
>      type: "arrow",
>      consumeOutsideClicks: true,
> -    useXulWrapper: true,

We can't just remove the xulWrapper here. It will force the tooltips to be constrained to the toolbox container, which is not OK with the current design for the editor tooltips (they will be cropped and unusable in too many cases)

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1301342.patch [1.0]

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8796425 [details] [diff] [review]
1301342.patch [1.0]

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

Works for me if try is green! Thanks.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b031c3aaa47c

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/364027ef5febd20776beea1e14c7bccefacab8f1
Bug 1301342 - Set noautohide for the HTMLTooltip panel and hide the overridden property search when editing a rule  r=jdescottes

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for timing out in devtools/client/shared/test/browser_html_tooltip-03.js:

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=364027ef5febd20776beea1e14c7bccefacab8f1
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=11779878&repo=fx-team

14:31:08     INFO -  1818 INFO TEST-PASS | devtools/client/shared/test/browser_html_tooltip-03.js | Focus is in the #box4-input -
14:31:08     INFO -  1819 INFO Test a tooltip without autofocus will not take focus
14:31:08     INFO -  1820 INFO Console message: [JavaScript Warning: "Property contained reference to invalid variable.  Error in parsing value for ‘filter’.  Falling back to ‘initial’." {file: "chrome://devtools/skin/tooltips.css" line: 121 column: 3020 source: " drop-shadow(0 3px 4px var(--theme-tooltip-shadow))"}]
14:31:08     INFO -  1821 INFO TEST-PASS | devtools/client/shared/test/browser_html_tooltip-03.js | Focus is still in the #box4-input -
14:31:08     INFO -  1822 INFO Console message: [JavaScript Warning: "Property contained reference to invalid variable.  Error in parsing value for ‘background-color’.  Falling back to ‘initial’." {file: "chrome://devtools/skin/tooltips.css" line: 144 column: 3388 source: " var(--theme-tooltip-background)"}]
14:31:08     INFO -  1823 INFO Waiting for event: 'blur' on [object XULElement].
14:31:08     INFO -  1824 INFO TEST-UNEXPECTED-FAIL | devtools/client/shared/test/browser_html_tooltip-03.js | Test timed out -

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6798c2644d2d

Comment 10

[Julian Descottes [:jdescottes]]

As discussed during triage, I'll try to investigate the failures on try.

Comment 11

[Julian Descottes [:jdescottes]]

I can not reproduce the failure on try locally. Try is using ubuntu 12.04, while locally I have 14.04.
Searching through old bugs I found Bug 771169 which seems to have stumbled upon the same issue.

Basically with some window managers, the window manager ignores noautofocus=true if noautohide=true (which is exactly what the test asserts here ...).

The workaround used in the bug was to rely on tooltips instead of panels. Try push doing just that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d58d5eef20e53e034915be26414d1e82d79643ce (not sure if we'll want to run with this though).

Comment 12

[Julian Descottes [:jdescottes]]

Try push using xul:Tooltip instead of xul:Panel is free from the timeout issue on html_tooltip-03.js.

However I don't really feel confident in switching from a xul:Panel to xul:Tooltip. The Tooltip seems to behave slightly differently from a Panel, I had to override paddings and line-height to reach a similar display. with small issues remaining, and that's only after testing manually on one platform for a few minutes). 

I propose we disable this particular test on Linux for now and keep using the xul:Panel. The downside is that the focus behavior will be incorrect on some Linux distributions. Since this is not a breaking behavior and it will only impact a small number of users I thing this is acceptable.

Comment 13

[Julian Descottes [:jdescottes]]

Try push skipping this specific test on Linux+xul-wrapper: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e23ad904450fc40c20c453bc4635f7ca0ea95f8f

Comment 14

[Julian Descottes [:jdescottes]]

Subsequent tests are failing, so skipping xul wrapper tests from tooltip-03 on Linux : https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce6e83672391150e4b7508f1d63fc88a4625d168

Comment 15

[Julian Descottes [:jdescottes]]

Attachment: Bug 1301342 - add noautohide=true on XUL panel wrapper for HTMLTooltip;

Review commit: https://reviewboard.mozilla.org/r/85844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85844/

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8801046 [details]
Bug 1301342 - add noautohide=true on XUL panel wrapper for HTMLTooltip;

https://reviewboard.mozilla.org/r/85844/#review84548

r+ assuming the mentioned change. There was also an extra change in my original patch that adds "this.filterProperty.hidden = true;" when we start editing. Please make sure that also lands.

::: devtools/client/shared/test/browser_html_tooltip-03.js:47
(Diff revision 1)
>    yield runTests(doc);
>  
>    info("Run tests for a Tooltip with a XUL panel");
>    useXulWrapper = true;
> +
> +  let isLinux = Services.appinfo.OS === "Linux";

We typically skip tests using a 'skip-if = os == "linux"' in the browser.ini file. I would follow the same pattern here.

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8801046 [details]
Bug 1301342 - add noautohide=true on XUL panel wrapper for HTMLTooltip;

https://reviewboard.mozilla.org/r/85844/#review84548

> We typically skip tests using a 'skip-if = os == "linux"' in the browser.ini file. I would follow the same pattern here.

I want to keep running the test on Linux when we are not using a XUL Wrapper, so I'd prefer to avoid skip-if here.

Comment 18

[Julian Descottes [:jdescottes]]

Attachment: Bug 1301342 - hide filter icon when editing property in ruleview;

Review commit: https://reviewboard.mozilla.org/r/85920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85920/

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8801151 [details]
Bug 1301342 - hide filter icon when editing property in ruleview;

https://reviewboard.mozilla.org/r/85920/#review84604

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8801046 [details]
Bug 1301342 - add noautohide=true on XUL panel wrapper for HTMLTooltip;

https://reviewboard.mozilla.org/r/85844/#review84548

> I want to keep running the test on Linux when we are not using a XUL Wrapper, so I'd prefer to avoid skip-if here.

Sounds good. Marking issue as fixed.

Comment 21

[Julian Descottes [:jdescottes]]

Thanks! Try looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c63bfb02d0db97ab4461c4f56c3b80ca8e8802b7&selectedJob=29180399 

Landing.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d03e0b83c81
add noautohide=true on XUL panel wrapper for HTMLTooltip;r=gl
https://hg.mozilla.org/integration/autoland/rev/81aa3da9ec82
hide filter icon when editing property in ruleview;r=gl

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8d03e0b83c81
https://hg.mozilla.org/mozilla-central/rev/81aa3da9ec82

Comment 24

[Saheda Reza [:Antora]]

I have reproduced this bug on firefox nightly according to(2016-09-08)

Fixing bug is verified on Latest Nightly -- Build ID:( 20161022030204 ),User Agent: Mozilla/5.0 (Windows NT 10.0; rv:52.0) Gecko/20100101 Firefox/52.0

Tested OS-- Windows10 32bit