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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Cristian Fogel [:cfogel]]

Attachment: devTools_inspectorWikipediaBodyOverflowX.gif

[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

[Patrick Brosset <:pbro>]

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

Comment 2

[Razvan Caliman [:rcaliman]]

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

Comment 3

[Razvan Caliman [:rcaliman]]

> 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

Comment 4

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1512956 - Ensure empty string is considered valid CSS authoredText; r=pbro

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.

Comment 5

[Razvan Caliman [:rcaliman]]

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

Comment 6

[Andrew Overholt [:overholt]]

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

Comment 7

[Razvan Caliman [: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.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 9

[Razvan Caliman [:rcaliman]]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/070cb0982606
Ensure empty string is considered valid CSS authoredText; r=pbro

Comment 11

[Razvan Caliman [:rcaliman]]

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

Comment 12

[Razvan Maries]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1293aa5ed192
Ensure empty string is considered valid CSS authoredText; r=pbro

Comment 14

[Razvan Caliman [:rcaliman]]

Fixed the issue and pushed again.
Try looks good this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50eb368e405736873588e50548d465bc80a29691&selectedJob=230261451

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/1293aa5ed192