Closed Bug 1297383 Opened 5 years ago Closed 5 years ago

Selector highlighter in the rule-view should also work with the 'element {}' rule

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: pbro, Assigned: sblin, Mentored)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

STR:
- open the inspector on this page
- in the rule-view, notice how each rule selector has a little inspector icon next to it
- click on it to see how this is useful to highlight all elements that this selector matches (you can search for .bz_comment_text elements for instance on this page)
- now, notice how there is also an element {} rule at the top of the rule-view, which allows to add properties just to the selected element, by using inline styles.

The goal of this bug is to also add the inspector icon next to the element {} rule, so that you can highlight the current element.

This is useful when you want to lock the highlighter on just one element and see its margin while you make changing for example.
Hi!

After a long time and a very intense year, I come back and I'd like to work on this bug.

The code is located in ~/Projets/Mozilla/mozilla-central/devtools/client/inspector/rules/views/rule-editor.js
Attached patch 1297383_1.patch (obsolete) — Splinter Review
Attachment #8791811 - Flags: review?(pbrosset)
Comment on attachment 8791811 [details] [diff] [review]
1297383_1.patch

Review of attachment 8791811 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch. This is a good start.

I found one bug that we'll need to fix before this lands:
- highlight an element{}
- then highlight another selector that the current element matches
- then click again to hide the selector highlight
-> the element{} is back to being highlighted, where it should not.
In fact, everytime you click on one of the inspector icons in the rule-view, that should unhighlight any other node or nodes that has/have been highlighted before.

Also, there are several ESLint errors in the code (simple formatting issues) that you need to fix too. We use ESLint to make sure our code looks consistent and ESLint runs automatically when a patch gets checked-in, so to avoid your code from being backed-out, please address these issues. You'll find everything you need to know about this here: https://wiki.mozilla.org/DevTools/CodingStandards but the simplest solution is to run `./mach eslint path/to/dir/or/file` locally so you know if there are errors.

And I see that your approach was to essentially duplicate the code path so that we would be able to deal both with selectors, and with elements. I think it might be a lot simpler if we kept just one code path: just deal with selectors. Now the idea would be to find a selector for the element{} rule. It turns out we can do that by using: this.ruleView.inspector.selectionCssSelector. This getter returns a *unique* CSS selector for the current element. So if that element has an ID for instance, then it will return #theId, otherwise, it will craft something that uniquely identifies the element.
To do this, I think one simple way would be in rule-editor.js (inside _create), instead of getting the selector with:
let selector = this.rule.domRule.selectors.join(", ");
get it with something like:
let selector = this.rule.domRule.selectors
               ? this.rule.domRule.selectors.join(", ")
               : this.ruleView.inspector.selectionCssSelector;
This way, nothing else needs to change.

Final comment: we'll need a new test for this feature, so we can be alerted in the future if we break it.
As you can see in devtools/client/inspector/rules/test/ we already have 3 tests for the selector highlighter feature:
browser_rules_selector-highlighter_01.js, browser_rules_selector-highlighter_02.js and browser_rules_selector-highlighter_03.js
I think you should add a 4th one: browser_rules_selector-highlighter_04.js which job is simply to check that the icon appears next to element{} rules too, and that it can be clicked.
Attachment #8791811 - Flags: review?(pbrosset)
Assignee: nobody → amarok
Status: NEW → ASSIGNED
Attached patch 1297383_v2.patch (obsolete) — Splinter Review
Thank you for the review!

I made some modifications (use selectionCssSelector, write a test, run eslint) to the patch
Attachment #8791811 - Attachment is obsolete: true
Attachment #8794431 - Flags: review?(pbrosset)
Comment on attachment 8794431 [details] [diff] [review]
1297383_v2.patch

Review of attachment 8794431 [details] [diff] [review]:
-----------------------------------------------------------------

This works well for me, and thanks for the new test.
I have made a number of minor comments below. Please take a look and ping me for a final review when you have a new patch ready.

::: devtools/client/inspector/rules/test/browser_rules_selector-highlighter_04.js
@@ +43,5 @@
> +  let icon = getRuleViewSelectorHighlighterIcon(view, "element");
> +
> +  yield clickSelectorIcon(icon, view);
> +  is(HighlighterFront.nodeFront.tagName, "P",
> +  "The right NodeFront is passed to the highlighter (1)");

nit: we usually indent wrapped lines like this:

is(HighlighterFront.nodeFront.tagName, "P",
   "The right NodeFront is passed to the highlighter (1)");

so that the argument on the second line is vertically aligned with the argument on the 1st line.

@@ +45,5 @@
> +  yield clickSelectorIcon(icon, view);
> +  is(HighlighterFront.nodeFront.tagName, "P",
> +  "The right NodeFront is passed to the highlighter (1)");
> +  is(HighlighterFront.options.selector, "body > p:nth-child(1)",
> +  "The right selector option is passed to the highlighter (1)");

Same here.

@@ +52,5 @@
> +  yield clickSelectorIcon(icon, view);
> +  ok(!HighlighterFront.isShown, "The toggle event says the highlighter is not visible");
> +});
> +
> +function* clickSelectorIcon(icon, view) {

This function exists in 3 tests:
browser_rules_selector-highlighter_02.js
browser_rules_selector-highlighter_03.js
browser_rules_selector-highlighter_04.js
It's time to move it to a common file and kill the duplication.
In this case, you can simply move this function to devtools/client/inspector/rules/test/head.js and remove it from these 3 files.

::: devtools/client/inspector/rules/views/rule-editor.js
@@ +150,5 @@
>      }
>  
> +    if ((this.rule.domRule.type !== CSSRule.KEYFRAME_RULE &&
> +      this.rule.domRule.selectors) ||
> +      (this.rule.domRule.type === ELEMENT_STYLE)) {

This condition can be made simpler. If a rule has no selectors, then it means it is the element style rule. So:

if (this.rule.domRule.type !== CSSRule.KEYFRAME_RULE)

is enough.
Attachment #8794431 - Flags: review?(pbrosset)
Attached patch 1297383_3.patch (obsolete) — Splinter Review
Hi!
Thanks for all.

I modified the patch.

Have a nice day,
Attachment #8794431 - Attachment is obsolete: true
Attachment #8795401 - Flags: review?(pbrosset)
Comment on attachment 8795401 [details] [diff] [review]
1297383_3.patch

Review of attachment 8795401 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Thanks.
I've pushed this to TRY to make sure all tests still pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45897a0d7131

One last thing, bugzilla tells me that your commit message is:
Bug 1297383 - Highlight current node via the selector highlighter for rule 'element {}'. r=pbro
Which is good, but when I apply your patch locally and run hg log, then I see the commit message being:
Bug 1297383 - "Selector highlighter in the rule-view should also work with the 'element {}' rule" [r=amarok]
Which isn't great. 
Can you change it? 
If you're using MQ, then hg qref -m "Bug 1297383 - Highlight current node via the selector highlighter for rule 'element {}'. r=pbro" would work
If not, then hg histedit -r . will let you change the message in an editor.
Attachment #8795401 - Flags: review?(pbrosset) → review+
Attached patch 1297383_3.patchSplinter Review
Hmm, can you try with this file? I see the correct message when I do a `hg log`
Attachment #8795401 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Attachment #8795702 - Flags: review+
(In reply to Sébastien Blin [:sblin] [:amarok] from comment #8)
> Created attachment 8795702 [details] [diff] [review]
> 1297383_3.patch
> 
> Hmm, can you try with this file? I see the correct message when I do a `hg
> log`
Ok thanks. 
Try is green now, so let's ask for a check-in. Thanks for having worked on this! This should land in FF 52.
Flags: needinfo?(pbrosset)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/182fd3e63603
Highlight current node via,the selector highlighter for rule 'element {}'. r=pbro
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/182fd3e63603
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
The related screenshots at https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS (and maybe also related pages) should be updated to reflect this change.

Sebastian
Keywords: dev-doc-needed
I didn't think it made sense to update all the screenshots for this change, but I've documented this feature here: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#element_rule.

Marking as dev-doc-complete, but let me know if I've missed anything.
(In reply to Will Bamberg [:wbamberg] from comment #13)
> I didn't think it made sense to update all the screenshots for this change,
> but I've documented this feature here:
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Examine_and_edit_CSS#element_rule.
> 
> Marking as dev-doc-complete, but let me know if I've missed anything.

Looks good to me, and it's even with a video, yay! Thank you, Will!

Sebastian
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.