1328016 - Ruleview breaks if there're invalid CSS (all rules are duplicated; editing works for wrong rules)

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open url [1]
2. Open devtools -> inspector -> ruleview, inspect <div>
3. Click on text "50px" in line "width: 50px;" to start editing the value
4. Press Shift+Up to change value from "50px" to "60px"

AR:  The black rectangle moves by 10px to the right (i.e. browser changes "left" instead of "width")
ER:  The black rectangle should increase width by 10px


STR_2:
1. Open url [1]
2. Open devtools -> inspector -> ruleview, inspect <div>
3. Click on text "translateY(0px)" to start editing the value
4. Type "gray"

AR:  The black rectangle becomes gray (i.e. browser changes "background" instead of "transform")
ER:  No visible changes


STR_3:
1. Open url [1]
2. Open devtools -> inspector -> ruleview, inspect <div>
3. {Click on text "50px" in line "height: 50px;". Press Escape to cancel} several times (N times)
4. Switch to another node and back
5. Open style editor

AR:  CSS rules in ruleview and style editor are duplicated N times. I.e. there're 6*2^N rules
ER:  No visible changes


> [1] data:text/html,<div><style>div{position:absolute;top50px;height:50px;left:50px;width:50px;background:black;transform:translateY(0px)}@ Tom Tromey :tromey:
It seems that this is a regresion caused by your change. Please have a look.

Comment 1

[Tom Tromey :tromey]

The bug here is that Rule._getTextProperties filters out properties that have no name
(according to parseDeclarations), but other code does not.

Comment 2

[Tom Tromey :tromey]

I think the cleanest way to deal with this is probably to filter out nameless properties
in parseDeclarations.

Comment 3

[Tom Tromey :tromey]

Attachment: Bug 1328016 - add parseNamedDeclarations and use in rule view;

Review commit: https://reviewboard.mozilla.org/r/101934/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101934/

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8823401 [details]
Bug 1328016 - add parseNamedDeclarations and use in rule view;

https://reviewboard.mozilla.org/r/101934/#review102838

Looks good to me.

::: devtools/shared/css/parsing-utils.js:458
(Diff revision 1)
> + * have a name.
> + */
> +function parseNamedDeclarations(isCssPropertyKnown, inputString,
> +                                parseComments = false) {
> +  return parseDeclarations(isCssPropertyKnown, inputString, parseComments)
> +    .filter((item) => !!item.name);

nit: I would perhaps vertically align the second line with the start of parseDeclaration.
And maybe remove the () around item argument in the arrow function.

Comment 5

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 6

[Tom Tromey :tromey]

Comment on attachment 8823401 [details]
Bug 1328016 - add parseNamedDeclarations and use in rule view;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101934/diff/1-2/

Comment 7

[Tom Tromey :tromey]

Fix an eslint nit.

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8823401 [details]
Bug 1328016 - add parseNamedDeclarations and use in rule view;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101934/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acaf6a89c4dd
add parseNamedDeclarations and use in rule view; r=pbro

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/acaf6a89c4dd

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Worth considering for uplift to Aurora/Beta? If so, please nominate accordingly :)

Comment 12

[Tom Tromey :tromey]

After thinking about it a while, I feel that this is a low-impact bug.
It only affects users with a certain kind of invalid CSS.  So, I think I
will not nominate it.