Rule view marks new properties as overridden

RESOLVED FIXED in Firefox 34

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: miker, Assigned: miker)

Tracking

({regression})

unspecified
Firefox 34
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
Keywords: regression
Assignee: nobody → mratcliffe
Comment hidden (obsolete)
Assignee: nobody → mratcliffe
Created attachment 8459315 [details] [diff] [review]
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.
Attachment #8459315 - Flags: review?(bgrinstead)
Status: NEW → ASSIGNED
Created attachment 8459335 [details] [diff] [review]
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
Attachment #8459315 - Attachment is obsolete: true
Attachment #8459315 - Flags: review?(bgrinstead)
Attachment #8459335 - Flags: review?(bgrinstead)
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 = "".
Attachment #8459335 - Flags: review?(bgrinstead) → review+
Created attachment 8459347 [details] [diff] [review]
rule-view-new-properties-always-marked-overridden-1040697.patch

Changed comment as requested.
Attachment #8459335 - Attachment is obsolete: true
Attachment #8459347 - Flags: review+
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
Attachment #8459347 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/0859e0ab3934
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34

Comment 9

4 years ago
Since this has landed, Firefox Desktop freezes when debugging some Firefox OS apps. See bug 1045616
QA Whiteboard: [qa-]
bug 1045616 fixed the problem with freezing.
Duplicate of this bug: 1032108
You need to log in before you can comment on or make changes to this bug.