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

RESOLVED FIXED in Firefox 42

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bgrins, Assigned: gl)

Tracking

Trunk
Firefox 42
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [rule-view-correctness])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Whiteboard: [rule-view-correctness]
(Assignee)

Updated

3 years ago
Assignee: nobody → gabriel.luong
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8639989 [details] [diff] [review]
1188125.patch
(Assignee)

Comment 2

3 years ago
Created attachment 8640089 [details] [diff] [review]
1188125.patch [1.0]

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)
(Reporter)

Comment 3

3 years ago
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)
(Assignee)

Comment 4

3 years ago
Created attachment 8640181 [details] [diff] [review]
1188125.patch [2.0]
Attachment #8640089 - Attachment is obsolete: true
Attachment #8640181 - Flags: review?(bgrinstead)
(Reporter)

Comment 6

3 years ago
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+
(Assignee)

Updated

3 years ago
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
(Assignee)

Comment 7

3 years ago
Created attachment 8640196 [details] [diff] [review]
1188125.patch [3.0]
Attachment #8640181 - Attachment is obsolete: true
Attachment #8640196 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/60a0b1f2e41c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.