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

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

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.

Comment 1

[Sébastien Blin [:sblin] [:amarok]]

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

Comment 2

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1297383_1.patch

Comment 3

[Patrick Brosset <:pbro>]

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.

Comment 4

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1297383_v2.patch

Thank you for the review!

I made some modifications (use selectionCssSelector, write a test, run eslint) to the patch

Comment 5

[Patrick Brosset <:pbro>]

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.

Comment 6

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1297383_3.patch

Hi!
Thanks for all.

I modified the patch.

Have a nice day,

Comment 7

[Patrick Brosset <:pbro>]

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.

Comment 8

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1297383_3.patch

Hmm, can you try with this file? I see the correct message when I do a `hg log`

Comment 9

[Patrick Brosset <:pbro>]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/182fd3e63603
Highlight current node via,the selector highlighter for rule 'element {}'. r=pbro

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/182fd3e63603

Comment 12

[Sebastian Zartner [:sebo]]

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

Comment 13

[Will Bamberg [:wbamberg]]

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.

Comment 14

[Sebastian Zartner [:sebo]]

(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