Closed Bug 1512956 Opened 9 months ago Closed 6 months ago

Wikipedia's body overflow-x property re-appears on inspector tab swap

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox-esr60 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

RESOLVED FIXED
Firefox 67
Tracking Status
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;
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).
Whiteboard: [dt-q]
(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: nobody → rcaliman
Status: NEW → ASSIGNED
Priority: -- → P2
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.
Has Regression Range: --- → yes

We were going through bugs tagged as regressions and saw this one. Is it just a matter of landing, Razvan?

Flags: needinfo?(rcaliman)

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.

Flags: needinfo?(rcaliman)

Razvan, any luck? Are you still working on this?

(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.

Flags: needinfo?(rcaliman)
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/070cb0982606
Ensure empty string is considered valid CSS authoredText; r=pbro

I think I finally fixed the outstanding issues. The try run looks ok aside from some unrelated intermittent failures. Attempting to land.

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

Flags: needinfo?(rcaliman)
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1293aa5ed192
Ensure empty string is considered valid CSS authoredText; r=pbro
Flags: needinfo?(rcaliman)
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.