Closed Bug 1188125 Opened 4 years ago Closed 4 years ago

[Rule View] Properties marked overridden does not take into account disabled properties

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: bgrins, Assigned: gl)

References

Details

(Whiteboard: [rule-view-correctness])

Attachments

(1 file, 3 obsolete files)

STR:

Open data:text/html,<style> body {background: green !important } body {background: red}</style><body></body>
Open rule view, uncheck the !important rule

Expected:
The red rule becomes un-crossed-out

Actual:
The red rule stays crossed-out
Whiteboard: [rule-view-correctness]
Assignee: nobody → gabriel.luong
Status: NEW → ASSIGNED
Attached patch 1188125.patch (obsolete) — Splinter Review
Attached patch 1188125.patch [1.0] (obsolete) — Splinter Review
I split up browser_ruleview_override.js into 4 separate test cases.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9ec63685ee4
Attachment #8639989 - Attachment is obsolete: true
Attachment #8640089 - Flags: review?(bgrinstead)
Comment on attachment 8640089 [details] [diff] [review]
1188125.patch [1.0]

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

::: browser/devtools/styleinspector/rule-view.js
@@ +333,5 @@
>      // determine if the property is overridden.
>      let textProps = [];
>      for (let rule of this.rules) {
>        if (rule.pseudoElement == pseudo && !rule.keyframes) {
> +        for (let textProp of rule.textProps) {

This isn't reversing the list so it ends up uncrossing out the wrong one when you add these to the same rule:

background: red;
background: green;

It'd be nice to encode this behavior into one of the new tests also (of course you'll need to manually add the two rules with the same name within the test for now since we don't have authored styles)

::: browser/devtools/styleinspector/test/browser_ruleview_mark_overridden_03.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +// Tests that the rule view marks overridden rules correctly based on the
> +// priority fo the rule

Typo: 'for'
Attachment #8640089 - Flags: review?(bgrinstead)
Attached patch 1188125.patch [2.0] (obsolete) — Splinter Review
Attachment #8640089 - Attachment is obsolete: true
Attachment #8640181 - Flags: review?(bgrinstead)
Comment on attachment 8640181 [details] [diff] [review]
1188125.patch [2.0]

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

Patch just needs a commit message
Attachment #8640181 - Flags: review?(bgrinstead) → review+
Summary: After unchecking a rule in the rule view, the newly applied rule doesn't get applied → [Rule View] Properties marked overridden does not take into account disabled properties
Attachment #8640181 - Attachment is obsolete: true
Attachment #8640196 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/60a0b1f2e41c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.