Closed
Bug 1142206
Opened 9 years ago
Closed 9 years ago
inspector strikes through CSS variable definitions
Categories
(DevTools :: Inspector, defect)
Tracking
(firefox42 fixed)
RESOLVED
FIXED
Firefox 42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: tromey, Assigned: tromey)
References
Details
Attachments
(2 files, 5 obsolete files)
186 bytes,
text/html
|
Details | |
9.86 KB,
patch
|
tromey
:
review+
|
Details | Diff | Splinter Review |
Open the attached HTML and then open the inspector. In the rule view, the variable definition has a line through it. This seems incorrect. Instead, I think the variable should only have a line through it if it is truly invalid in some way; for example either it has an unparseable value, or perhaps if it participates in a use/definition cycle.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ttromey
Assignee | ||
Comment 1•9 years ago
|
||
This patch fixes the bug by simply checking to see if the property in question is a CSS variable definition.
Assignee | ||
Updated•9 years ago
|
Attachment #8576248 -
Flags: review?(pbrosset)
Assignee | ||
Updated•9 years ago
|
Attachment #8576248 -
Flags: review?(pbrosset)
Assignee | ||
Comment 2•9 years ago
|
||
Oops, I was too hasty requesting review. Somehow between filing this and hacking on it I forgot that I still wanted to make the variable display as invalid if it has some problem. How embarrassing.
Assignee | ||
Comment 3•9 years ago
|
||
Here's a new version that actually works. This changes inDOMUtils::GetSubpropertiesForCSSProperty to treat custom properties like other properties.
Attachment #8576248 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8634912 [details] [diff] [review] let GetSubpropertiesForCSSProperty handle custom property Not sure if this requires two reviews; but just in case, one for the inDOMUtils change and one for the test case.
Attachment #8634912 -
Flags: review?(pbrosset)
Attachment #8634912 -
Flags: review?(cam)
Comment 5•9 years ago
|
||
Comment on attachment 8634912 [details] [diff] [review] let GetSubpropertiesForCSSProperty handle custom property Review of attachment 8634912 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the inDOMUtils.cpp changes with the following: ::: layout/inspector/inDOMUtils.cpp @@ +627,2 @@ > return NS_ERROR_FAILURE; > + } else if (propertyID == eCSSPropertyExtra_variable) { Make this a separate if statement (like the one below) rather than an else if.
Attachment #8634912 -
Flags: review?(cam) → review+
Comment 6•9 years ago
|
||
Comment on attachment 8634912 [details] [diff] [review] let GetSubpropertiesForCSSProperty handle custom property Review of attachment 8634912 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/styleinspector/test/browser_ruleview_custom.js @@ +28,5 @@ > +function* removeTestContent(inspector, node) { > + let onMutated = inspector.once("markupmutation"); > + node.remove(); > + yield onMutated; > +} The createTestContent and removeTestContent functions rely on CPOWs a lot: - 'content' is used, - addStyle is used which manipulated nodes in content, - innerHTML - node.remove(); This works, but we're trying to move away from it because: - the e10s people say it's bad and warnings are emitted in the logs when we do use CPOWs, - there's an ongoing devtools effort to try and run tests remotely on simulators or distant devices where direct access to content through CPOWs is just not possible. I suggest creating a new HTML test page in the test directory, reference it in browser.ini and load it here instead. This page could contain several nodes so that you can select each one, in turn, before running the next test step.
Attachment #8634912 -
Flags: review?(pbrosset)
Assignee | ||
Comment 7•9 years ago
|
||
Updated per review.
Attachment #8634912 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8635408 [details] [diff] [review] let GetSubpropertiesForCSSProperty handle custom property Sorry about that Patrick. This one should be cleaner.
Attachment #8635408 -
Flags: review?(pbrosset)
Assignee | ||
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=859b8ae72959
Updated•9 years ago
|
Attachment #8635408 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 10•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=23cffa5f22e1
Assignee | ||
Comment 11•9 years ago
|
||
Rebased.
Assignee | ||
Updated•9 years ago
|
Attachment #8636043 -
Flags: review+
Assignee | ||
Comment 12•9 years ago
|
||
Fix test failure.
Assignee | ||
Comment 13•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f582aadc38a3
Assignee | ||
Updated•9 years ago
|
Attachment #8636043 -
Attachment is obsolete: true
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8636170 [details] [diff] [review] let GetSubpropertiesForCSSProperty handle custom property Testing revealed that I had missed layout/inspector/tests/test_bug1046140.html. That test, naturally, comes from bug 1046140. It turns out that this patch essentially rewrites the same code as that patch to do something else. Modifying that test case in-place seemed a bit odd, since the bug number is baked into the test filename, but I would be modifying the test to do something totally different. So, I chose to remove the test and instead put a little addition into a related test. Requesting re-review in case this isn't the normal thing to do.
Attachment #8636170 -
Flags: review?(cam)
Assignee | ||
Comment 15•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=099e44297e9a
Assignee | ||
Comment 16•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e74cde4b500d
Comment 17•9 years ago
|
||
Comment on attachment 8636170 [details] [diff] [review] let GetSubpropertiesForCSSProperty handle custom property Review of attachment 8636170 [details] [diff] [review]: ----------------------------------------------------------------- Replacing test_bug1046140.html with the addition to test_bug1006595.html is fine. r=me on the layout/ changes.
Attachment #8636170 -
Flags: review?(cam) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Forgot to update mochitest.ini.
Assignee | ||
Updated•9 years ago
|
Attachment #8635408 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8636170 -
Attachment is obsolete: true
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8636321 [details] [diff] [review] let GetSubpropertiesForCSSProperty handle custom property Carrying over r+
Attachment #8636321 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0743be2712b6
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0743be2712b6
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•