Closed
Bug 1301342
Opened 8 years ago
Closed 8 years ago
Class "ruleview-overridden" is gone after opening the color picker from color property of element which contains it
Categories
(DevTools :: Inspector: Rules, defect, P1)
DevTools
Inspector: Rules
Tracking
(firefox49 unaffected, firefox50 affected, firefox51 affected, firefox52 fixed)
RESOLVED
FIXED
Firefox 52
Tracking | Status | |
---|---|---|
firefox49 | --- | unaffected |
firefox50 | --- | affected |
firefox51 | --- | affected |
firefox52 | --- | fixed |
People
(Reporter: magicp.jp, Assigned: jdescottes)
References
Details
Attachments
(4 files, 1 obsolete file)
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.
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
Updated•8 years ago
|
Assignee: nobody → gl
status-firefox52:
--- → affected
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 2•8 years ago
|
||
Attachment #8794373 -
Flags: review?(jdescottes)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
Attachment #8794373 -
Attachment is obsolete: true
Attachment #8796425 -
Flags: review?(jdescottes)
Assignee | ||
Comment 5•8 years ago
|
||
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+
Comment 6•8 years ago
|
||
Comment 7•8 years ago
|
||
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•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(gl)
Comment 9•8 years ago
|
||
Assignee | ||
Comment 10•8 years ago
|
||
As discussed during triage, I'll try to investigate the failures on try.
Assignee: gl → jdescottes
Assignee | ||
Comment 11•8 years ago
|
||
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).
Assignee | ||
Comment 12•8 years ago
|
||
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.
Assignee | ||
Comment 13•8 years ago
|
||
Try push skipping this specific test on Linux+xul-wrapper: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e23ad904450fc40c20c453bc4635f7ca0ea95f8f
Assignee | ||
Comment 14•8 years ago
|
||
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•8 years 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•8 years 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•8 years 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•8 years 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.
Assignee | ||
Comment 21•8 years ago
|
||
Thanks! Try looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c63bfb02d0db97ab4461c4f56c3b80ca8e8802b7&selectedJob=29180399
Landing.
Comment 22•8 years 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8d03e0b83c81
https://hg.mozilla.org/mozilla-central/rev/81aa3da9ec82
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment 24•8 years ago
|
||
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]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•