Closed Bug 1142206 Opened 7 years ago Closed 7 years ago

inspector strikes through CSS variable definitions

Categories

(DevTools :: Inspector, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

Attachments

(2 files, 5 obsolete files)

Attached file css-t2.html
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: nobody → ttromey
This patch fixes the bug by simply checking to see if the property in
question is a CSS variable definition.
Attachment #8576248 - Flags: review?(pbrosset)
Attachment #8576248 - Flags: review?(pbrosset)
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.
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
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 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 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)
Updated per review.
Attachment #8634912 - Attachment is obsolete: true
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)
Attachment #8635408 - Flags: review?(pbrosset) → review+
Attachment #8636043 - Flags: review+
Fix test failure.
Attachment #8636043 - Attachment is obsolete: true
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)
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+
Forgot to update mochitest.ini.
Attachment #8635408 - Attachment is obsolete: true
Attachment #8636170 - Attachment is obsolete: true
Comment on attachment 8636321 [details] [diff] [review]
let GetSubpropertiesForCSSProperty handle custom property

Carrying over r+
Attachment #8636321 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0743be2712b6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Duplicate of this bug: 1126702
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.