1040697 - Rule view marks new properties as overridden

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

STR:
1. From about:home inspect the nightly image.
2. Click inside element {here}
3. Type "display:inline" <enter>

display:inline will have a line through

STR 2:
1. From about:home inspect the nightly image.
2. Right click and "Add rule."
3. For the selector type div #brandLogo
3. Click inside the selector's curly braces.
4. Type "display:inline" <enter>

display:inline will have a line through

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Looks like I won't have time to fix this so feel free to take a look at it.

It seems like the way properties are added has changed and this has completely broken the crossing out of overridden properties. All new properties have overridden set to true so if we can find out why it should be easy to fix.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: rule-view-new-properties-always-marked-overridden-1040697.patch

Glad I created domUtils.getSubpropertiesForCSSProperty() as it is exactly what we needed to fix this issue.

In the past we processed everything twice to fix this but somebody noticed and removed that hack, this is much tidier.

The test fails without the rule-view changes so this should be it.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Try:
https://tbpl.mozilla.org/?tree=Try&rev=a016eb8fa8ca

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: rule-view-new-properties-always-marked-overridden-1040697.patch

Missing try / catch caused a failing test.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=89d20f35385c

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8459335 [details] [diff] [review]
rule-view-new-properties-always-marked-overridden-1040697.patch

Review of attachment 8459335 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleinspector/rule-view.js
@@ +992,5 @@
>      this.computed = [];
> +
> +    try {
> +      // Manually get the subProperties so that new properties (value == "")
> +      // are still added to this.computed.

This comment should be clearer.  There was some nuance with the bug (that dummyStyle was skipping properties with an empty value) that is quite tricky to understand.  Maybe something like this:

// Manually get all the properties that are set when setting a value on
// this.name and check the computed style on dummyElement for each one.
// If we just read dummyStyle, it would skip properties when value = "".

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: rule-view-new-properties-always-marked-overridden-1040697.patch

Changed comment as requested.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8459347 [details] [diff] [review]
rule-view-new-properties-always-marked-overridden-1040697.patch

https://hg.mozilla.org/integration/fx-team/rev/0859e0ab3934

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0859e0ab3934

Comment 9

[Paul Rouget [:paul]]

Since this has landed, Firefox Desktop freezes when debugging some Firefox OS apps. See bug 1045616

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

bug 1045616 fixed the problem with freezing.