Wikipedia's body overflow-x property re-appears on inspector tab swap
Categories
(DevTools :: Inspector, defect, P2)
Tracking
(firefox-esr60 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 fixed)
People
(Reporter: cfogel, Assigned: rcaliman)
Details
(Keywords: regression, Whiteboard: [dt-q])
Attachments
(2 files)
[Affected versions]: - 65.0a1 (2018-12-09), 64.0b14, 63.0.3 [Affected platforms]: - win10x64, Ubuntu 16.04, macOS 10.13 [Steps to reproduce]: 1. Launch Firefox; 2. Access https://www.wikipedia.org/ 3. Open the inspector; 4. Click on the body tag; 5. Delete the overflow-y property and click somewhere else; 6. Click on any tab from the inspector (Layout/Computed/Changes/etc.); 7. Click on the Rules tab; [Expected result]: - The body property is still removed; [Actual result]: - overflow-x: hidden; is listed under body in the rules section; [Regression range]: - last good: 2017-01-13 changeset: 91f5293e9a89056565493ed5073c3842b0ee9fdc - first bad: 2017-01-15 changeset: 5ce3882eec21be3a70e4afc050959ca2f76bfa76 [Additional notes]: - 3pane needs to be toggled off; - attached recording with the issue; - property is not re-added if the inspector is toggled off->on;
Comment 1•6 years ago
|
||
This can be reproduced with a minimal test case: 1. open data:text/html,<style>body{color:red}</style>test 2. in the rule-view, remove the color:red declaration (by deleting the property or value) 3. switch to another panel 4. go back to the rule-view panel It always happens when there is only 1 declaration in the rule. If there are several declarations, then the problem only starts occurring once you've removed all of them. The culprit is likely this line: https://searchfox.org/mozilla-central/rev/dc3585e58b427c3a8a7c01712fe54ebe118a3ce2/devtools/server/actors/styles.js#1250 where we check if the authoredText for a rule exists simply by checking if it is truthy. However, if it exists but is empty, then this will be falsy, and we will use the cssText property instead, causing the problem (since it hasn't been updated with the properties being removed).
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 2•6 years ago
|
||
(In reply to Patrick Brosset <:pbro> from comment #1) > This can be reproduced with a minimal test case: > > 1. open data:text/html,<style>body{color:red}</style>test > 2. in the rule-view, remove the color:red declaration (by deleting the > property or value) > 3. switch to another panel > 4. go back to the rule-view panel The issue reproduces even without switching to another panel. Just delete the single property, select a different node in the Markup view, then select the original node again. The deleted property is still there. > The culprit is likely this line: https://searchfox.org/mozilla-central/rev/dc3585e58b427c3a8a7c01712fe54ebe118a3ce2/devtools/server/actors/styles.js#1250 Thanks for the lead! Fixing just that does not solve the problem. I am investigating further.
Assignee | ||
Comment 3•6 years ago
|
||
> The culprit is likely this line: https://searchfox.org/mozilla-central/rev/dc3585e58b427c3a8a7c01712fe54ebe118a3ce2/devtools/server/actors/styles.js#1250 That was indeed the issue, but there was another similar correction to be made in the front: https://searchfox.org/mozilla-central/source/devtools/shared/fronts/styles.js#163
Assignee | ||
Comment 4•6 years ago
|
||
When removing all declarations from a rule via the Rule view, the authoredText value can be an empty string. This patch ensures that the fallback cssText is not used in that case since that unintendedly restores the whole declaration block when reparsing the text of the rule.
Assignee | ||
Comment 5•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1a9624480212606c4b666b088374a530314fed3
Updated•5 years ago
|
Comment 6•5 years ago
|
||
We were going through bugs tagged as regressions and saw this one. Is it just a matter of landing, Razvan?
Assignee | ||
Comment 7•5 years ago
|
||
The patch for this bug caused test failures which I have not yet investigated. I'll pick it up again and try to land it.
Comment 8•5 years ago
|
||
Razvan, any luck? Are you still working on this?
Assignee | ||
Comment 9•5 years ago
|
||
(In reply to Liz Henry (:lizzard) (use needinfo) from comment #8)
Razvan, any luck? Are you still working on this?
No progress yet; been working on other bugs. I'll give it my attention this week.
Comment 10•5 years ago
|
||
Pushed by rcaliman@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/070cb0982606 Ensure empty string is considered valid CSS authoredText; r=pbro
Assignee | ||
Comment 11•5 years ago
|
||
I think I finally fixed the outstanding issues. The try run looks ok aside from some unrelated intermittent failures. Attempting to land.
Comment 12•5 years ago
|
||
Backed out for xpcshell fails on devtools/client/shared/test/unit/test_parseDeclarations.js.
Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=070cb098260644cbd2cac29deee6ac56d0fd6fb9&selectedJob=230154549
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230154549&repo=autoland&lineNumber=12889
12:53:27 INFO - TEST-START | devtools/client/shared/test/unit/test_parseDeclarations.js
12:53:27 WARNING - TEST-UNEXPECTED-FAIL | devtools/client/shared/test/unit/test_parseDeclarations.js | xpcshell return code: 0
12:53:27 INFO - TEST-INFO took 419ms
12:53:27 INFO - >>>>>>>
12:53:27 INFO - PID 5628 | Unable to load \untrusted-startup-test-dll.dll; LoadLibraryW failed: 126[5628, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
12:53:27 INFO - PID 5628 | [5628, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
12:53:27 INFO - PID 5628 | [5628, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
12:53:27 INFO - PID 5628 | [5628, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
12:53:27 INFO - PID 5628 | [5628, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
12:53:27 INFO - PID 5628 | [5628, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file z:/build/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2528
12:53:27 INFO - PID 5628 | JavaScript strict warning: resource://devtools/shared/Loader.jsm, line 220: ReferenceError: reference to undefined property "name"
Backout: https://hg.mozilla.org/integration/autoland/rev/d87cc2cc36411fe6e5b2b6eccc210ed19eb88814
Comment 13•5 years ago
|
||
Pushed by rcaliman@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1293aa5ed192 Ensure empty string is considered valid CSS authoredText; r=pbro
Assignee | ||
Comment 14•5 years ago
|
||
Fixed the issue and pushed again.
Try looks good this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50eb368e405736873588e50548d465bc80a29691&selectedJob=230261451
Comment 15•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Updated•5 years ago
|
Description
•