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

VERIFIED FIXED in Firefox 44

Status

()

Firefox
Developer Tools: Inspector
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: magicp, Assigned: tromey)

Tracking

Trunk
Firefox 45
Points:
---

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8684249 [details]
Screen-movie-20151106184420.mp4

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

Updated

2 years ago
Has STR: --- → yes
status-firefox44: --- → affected
status-firefox45: --- → affected
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Updated

2 years ago
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
(Assignee)

Comment 1

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

Comment 2

2 years ago
Created attachment 8684327 [details] [diff] [review]
fix how rewriteDeclarations reports unrelated changes
(Assignee)

Comment 3

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

Comment 4

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=982eec313630
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 5

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/872da083afd5
Keywords: checkin-needed
(Assignee)

Comment 6

2 years ago
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?

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/872da083afd5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45

Comment 8

2 years ago
magicp, could you please verify this issue is fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(magicp.jp)

Comment 9

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

Comment 10

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

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d4159c66b183
status-firefox44: affected → fixed
(Reporter)

Updated

2 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 12

2 years ago
Sorry, I have mistaken changing status...

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d4159c66b183
status-b2g-v2.5: --- → fixed
(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!
status-b2g-v2.5: fixed → ---
You need to log in before you can comment on or make changes to this bug.