Closed Bug 1222461 Opened 4 years ago Closed 4 years ago

[ruleview] Do not duplicate !important rule when changed the ruleview property checkbox off/on twice

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox44 fixed, firefox45 fixed)

VERIFIED FIXED
Firefox 45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed

People

(Reporter: magicp.jp, Assigned: tromey)

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20151106004026

Steps to reproduce:

Please refer to attached movie
1. Open "www.yahoo.com"
2. Open Inspect Element > Rules
3. Select <body> element
4. Enter "important" in ruleview-searchbox
5. Change checkbox off/on twice of !important rule 


Actual results:

!important rule is duplicated "!important !important"


Expected results:

Do not duplicate !important rule.
Has STR: --- → yes
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
On the plus side, it is showing "!important !important", but not actually
writing that back to the style sheet.  So at least it isn't breaking the style sheet.
Comment on attachment 8684327 [details] [diff] [review]
fix how rewriteDeclarations reports unrelated changes

This bug was a bit sneaky.  It turned out that the rewriter was reporting
that even the currently-being-rewritten decl had changed; and this was
confusing things.  This patch fixes the problem by changing the rewriter
to (1) correctly identify changes, and (2) only report changes that affect
the property's actual value.

I changed the rewriter test to always test the changed declarations, to
avoid bugs of this nature in the future.
Attachment #8684327 - Flags: review?(pbrosset)
Attachment #8684327 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
Comment on attachment 8684327 [details] [diff] [review]
fix how rewriteDeclarations reports unrelated changes

Approval Request Comment
[Feature/regressing bug #]: 984880
[User impact if declined]:
Disabling and re-enabling a property marked "!important" will cause
"!important !important" to show up in the rule view.
[Describe test coverage new/current, TreeHerder]:
There is a new test for the new behavior; plus all the existing rewriter
tests were updated to verify that the "change" behavior reported by the
rewriter is correct in all cases.
[Risks and why]: 
While it is a subtle change, I think the new test coverage is more than
sufficient.
The main failure mode would be that the rewriter would fail to report a
rewritten declaration.  This would be confusing but a workaround is to
select some other element in the inspector, then revisit the element one
is interested in.
[String/UUID change made/needed]: No.
Attachment #8684327 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/872da083afd5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
magicp, could you please verify this issue is fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(magicp.jp)
Comment on attachment 8684327 [details] [diff] [review]
fix how rewriteDeclarations reports unrelated changes

This has been on Nightly for 2 days now, seems safe to uplift Aurora44.
Attachment #8684327 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Ritu Kothari (:ritu) from comment #8)
> magicp, could you please verify this issue is fixed as expected on the
> latest Nightly build? Thanks!

I have verified this issue was fixed as expected on the latest Nightly build (20151112030238). Thanks!
Flags: needinfo?(magicp.jp)
Status: RESOLVED → VERIFIED
Sorry, I have mistaken changing status...
(In reply to magicp from comment #10)
> (In reply to Ritu Kothari (:ritu) from comment #8)
> > magicp, could you please verify this issue is fixed as expected on the
> > latest Nightly build? Thanks!
> 
> I have verified this issue was fixed as expected on the latest Nightly build
> (20151112030238). Thanks!

Thanks. This is very helpful!
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.