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

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: CSS Rules Inspector
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: magicp, Assigned: jdescottes)

Tracking

Trunk
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 unaffected, firefox50 affected, firefox51 affected, firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8789300 [details]
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.
(Reporter)

Updated

a year ago
Blocks: 1267414
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox49: --- → unaffected
status-firefox50: --- → affected
status-firefox51: --- → affected
Component: Untriaged → Developer Tools: CSS Rules Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Assignee: nobody → gl
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P1
(Reporter)

Updated

a year ago
status-firefox52: --- → affected
Status: NEW → ASSIGNED
Created attachment 8794373 [details] [diff] [review]
1301342.patch
Attachment #8794373 - Flags: review?(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)
Attachment #8794373 - Flags: review?(jdescottes)
Created attachment 8796425 [details] [diff] [review]
1301342.patch [1.0]
Attachment #8794373 - Attachment is obsolete: true
Attachment #8796425 - Flags: review?(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.
Attachment #8796425 - Flags: review?(jdescottes) → review+
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
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 -
Flags: needinfo?(gl)
Flags: needinfo?(gl)
As discussed during triage, I'll try to investigate the failures on try.
Assignee: gl → 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).
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.
Subsequent tests are failing, so skipping xul wrapper tests from tooltip-03 on Linux : https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce6e83672391150e4b7508f1d63fc88a4625d168
Comment hidden (mozreview-request)

Comment 16

a year ago
mozreview-review
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.
Attachment #8801046 - Flags: review?(gl) → review+
(Assignee)

Comment 17

a year ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 19

a year ago
mozreview-review
Comment on attachment 8801151 [details]
Bug 1301342 - hide filter icon when editing property in ruleview;

https://reviewboard.mozilla.org/r/85920/#review84604
Attachment #8801151 - Flags: review?(gl) → review+

Comment 20

a year ago
mozreview-review-reply
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 22

a year ago
Pushed by jdescottes@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d03e0b83c81
https://hg.mozilla.org/mozilla-central/rev/81aa3da9ec82
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Depends on: 1310957
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
QA Whiteboard: [testday-20161021]
You need to log in before you can comment on or make changes to this bug.