1178535 - [Rule View] Highlight the selector's pseudoclass lock in the rule editor

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

When we toggle a pseudo class lock and the rule view updates to include the new rules, it is not immediately clear which new rule was added. 

Couple of ideas include:
(1) Adding a label.
(2) Changing the font color for the pseudo class in the selector text.

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

@tromey, Any idea on the best approach to this? Currently, I am thinking about using regexps to match the list of pseudo classes, but I suspect we don't want to do that.

https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-classes

Comment 2

[Tom Tromey :tromey]

(In reply to Gabriel Luong [:gl] from comment #1)
> @tromey, Any idea on the best approach to this? Currently, I am thinking
> about using regexps to match the list of pseudo classes, but I suspect we
> don't want to do that.
> 
> https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-classes

At the lexing level, these have different meanings:

x:active { }
x\:active { }

The first lexes as "ident, symbol, ident", whereas the second as
a single ident whose (post-processed) text is "x:active".

I am not certain but I think this probably affects whether a selector
is matching a pseudo-class (the first case) or just an ordinary
identifier with a funny name (the second case).

Assuming this is true what I would recommend is instantiating a CSSLexer
and writing a tiny parser to check for the forms that matter.

This may all be irrelevant depending on where you get your data from.
If you already know that you're looking at a selector with a pseudo-class,
and it can't possibly be some funny thing, then I think a regexp would be
perfectly reasonable.

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1178535-1.patch

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8628483 [details] [diff] [review]
1178535-1.patch

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

Hi Tom,

I was talking to bgrins about the possible direction we can go about parsing the CSS selector. Currently, parseSelector returns an array that breaks down the given selector input to chunks that we care about. The following is a couple examples:
"a:hover" -> ["a", ":hover"]
"#searchText + #searchSubmit:hover" -> ["#searchText", " ", "+", " ", "#searchSubmit", ":hover"]
"html:not([searchUIConfiguration="oldsearchui"]) #searchLogoContainer" -> ["html", ":not([searchUIConfiguration="oldsearchui"])", " ", "#searchLogoContainer"]
".bugzilla-mozilla-org.skin-Mozilla.bz_bug.bz_status_RESOLVED.bz_product_Firefox.bz_component_Developer_Tools\:_Inspector.bz_bug_1166959.bz_gravatar.yui-skin-sam" -> [".bugzilla-mozilla-org.skin-Mozilla.bz_bug.bz_status_RESOLVED.bz_product_Firefox.bz_component_Developer_Tools\:_Inspector.bz_bug_1166959.bz_gravatar.yui-skin-sam"]

The current problem is that the code is quite complex and you can see we have to do some funny things in order to escape characters from the token.text, and have to build up the original selector text from the tokens. There may be edge cases that have not been addressed yet where the result from parseSelector does not equal the initial given value. Do you know a better way about going about this?

We came up with a counter proposal to just parse out the pseudoclass locks we care about using the startOffset and endOffset in the token data. The follow is a couple examples:
"body.foo:hover.bar::before:hover" -> ["body.foo", ":hover", ".bar::before", ":hover"]
"body#id:not(:hover)" -> ["body#id:not(", ":hover", ")"]

In this case, we would not have to build up the selector text from the token text, but use the startOffset and endOffset to figure out how to splice the given selector text.

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d4a898d5ce9

Comment 6

[Tom Tromey :tromey]

(In reply to Gabriel Luong [:gl] from comment #4)

> The current problem is that the code is quite complex and you can see we
> have to do some funny things in order to escape characters from the
> token.text

Rather than re-escaping you can just use the original text.
That is, rather than CSS.escape(token.text), use
value.substring(token.startOffset, token.endOffset).

Also I think the patch has a bug in the case where whitespace appears
in function arguments.

> We came up with a counter proposal to just parse out the pseudoclass locks
> we care about using the startOffset and endOffset in the token data. The
> follow is a couple examples:
> "body.foo:hover.bar::before:hover" -> ["body.foo", ":hover", ".bar::before",
> ":hover"]
> "body#id:not(:hover)" -> ["body#id:not(", ":hover", ")"]
> 
> In this case, we would not have to build up the selector text from the token
> text, but use the startOffset and endOffset to figure out how to splice the
> given selector text.

Sorry about this, but I am not sure I really understand.
In the quoted paragraph I don't understand the relationship between
startOffset/endOffset and the issue around parsing.

So, sorry again if my reply isn't that useful.

One issue seems to be the contract of the new function.
I think it's totally fine to write an ad hoc function that returns data in
the form you need; just don't name it "parseSelector".

I would be tempted to write a parser for the CSS selector syntax
(http://www.w3.org/TR/css3-selectors/#selector-syntax) that returns
structured objects representing the selectors; however, without a second user, that
may be overkill.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Highlight Pseudoclass locks (active, hover, focus) - Dark Theme

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Highlight Pseudoclass locks (active, hover, focus) - Light Theme

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8629023 [details]
Highlight Pseudoclass locks (active, hover, focus) - Dark Theme

@shorlander, I attached the screenshots of the dark and light theme highlighting of the pseudoclass lock text such as :hover, :active, and :focus in the rule view selector text.

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1178535-1.patch [Parsing only pseudoclass locks]

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Patrick, how do you feel about highlighting all the attributes and pseudoclasses in the rule view? Currently, I am thinking we should apply a highlight for (1) pseudoclass locks that are applied when we toggle the states from the panel, (2) attributes, and (3) all the other pseudoclasses. 

The idea would be to have different highlight colours for (1), (2) and (3).

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1178535-1.patch [Parsing Attributes and Pseudoclasses]

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1178535-1.patch [Parsing Attributes and Pseudoclasses]

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Highlight pseudoclass lock text in the selector text of the rule view

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Parse the pseudoclasses and attributes from the selector text

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fc03f0fc817

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Highlight pseudoclass and attribute texts in the selector text of the rule view

- Removes an extraneous selector container span
- Fixes Copy Selector context menu

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83c0f2756e4f

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

The current try build highlights the pseudoclasses and attributes. There is a different color for pseudoclass locks that are applied from the pseudoclass panels.

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Highlight pseudoclass and attribute texts in the selector text of the rule view

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c74f95d52ecd

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Highlight Attribute

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Highlight other pseudoclasses

Comment 22

[Tom Tromey :tromey]

Comment on attachment 8629839 [details] [diff] [review]
Part 1: Parse the pseudoclasses and attributes from the selector text

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

Looks good -- one nit and one possible issue.  The test case is excellent, thanks.

::: browser/devtools/styleinspector/css-parsing-utils.js
@@ +90,5 @@
>    declarations = declarations.filter(prop => prop.name || prop.value);
>    return declarations;
> +}
> +
> +function parsePseudoClassesAndAttributes(value) {

This needs a documentation comment; see the other exported functions in the file for examples.

I think it should at least describe the meaning of the output and whether names in the output are escaped.

@@ +137,5 @@
> +          result.push({ value: current, type: SELECTOR_PSEUDO_CLASS });
> +        }
> +
> +        current = "";
> +        hasFunction = false;

I think this will fail if there is a function with nested parens, though I don't know if this is possible.  What about maybe x:not(b:nth-child(0)) -- is that valid?

Comment 23

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Parse the pseudoclasses and attributes from the selector text [1.0]

Thanks for the review Tom!

x:not(b:nth-child(0)) is indeed valid. Flagging you for another review because of that scenario.

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Parse the pseudoclasses and attributes from the selector text [2.0]

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Highlight pseudoclass and attribute texts in the selector text of the rule view [1.0]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7888b4855f3

Comment 26

[Tom Tromey :tromey]

Comment on attachment 8630877 [details] [diff] [review]
Part 1: Parse the pseudoclasses and attributes from the selector text [2.0]

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

Looks good.

::: browser/devtools/styleinspector/css-parsing-utils.js
@@ +95,5 @@
> +
> +/**
> + * Returns an array of the parsed CSS selector components (element, attributes
> + * and pseudoclasses) given an string.
> + *

I think this needs a bit more detail.

It should refer to the SELECTOR_* constants.

I didn't understand why ".testclass, #testid" resulted in a single SELECTOR_ELEMENT.  I think the docs explain that sort of thing.  And I wondered what would happen for ".testclass, #testid:hover"

Comment 27

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Parse the pseudoclasses and attributes from the selector text [3.0]

(In reply to Tom Tromey :tromey from comment #26)
> Comment on attachment 8630877 [details] [diff] [review]
> Part 1: Parse the pseudoclasses and attributes from the selector text [2.0]
> 
> Review of attachment 8630877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: browser/devtools/styleinspector/css-parsing-utils.js
> @@ +95,5 @@
> > +
> > +/**
> > + * Returns an array of the parsed CSS selector components (element, attributes
> > + * and pseudoclasses) given an string.
> > + *
> 
> I think this needs a bit more detail.
> 
> It should refer to the SELECTOR_* constants.

Updated the docs with more detail.

> 
> I didn't understand why ".testclass, #testid" resulted in a single
> SELECTOR_ELEMENT.  I think the docs explain that sort of thing.  And I
> wondered what would happen for ".testclass, #testid:hover"

This test case is actually a mistake. The selectors that would be fed into parsePseudoClassesAndAttributes would've been split into their individual selectors already.

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 8630500 [details]
Highlight other pseudoclasses

My 2 cents - I don't like the green here because it blends in too much with the property values below.  I'd prefer to see it just use the normal color or go with the same color that's used for pseudo class locks.

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8630912 [details] [diff] [review]
Part 2: Highlight pseudoclass and attribute texts in the selector text of the rule view [1.0]

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

Frontend changes seem fine.  I'm still a little worried that we may be getting the wrong data from parsePseudoClassesAndAttributes given some crazy selector as input, but I trust tromey's and your judgement there.

::: browser/themes/shared/devtools/ruleview.css
@@ +254,5 @@
> +  color: var(--theme-highlight-purple);
> +}
> +
> +.ruleview-selector-matched > .ruleview-selector-pseudo-class {
> +  color: var(--theme-highlight-green);

I don't like the green here personally, but let's see what Stephen says

Comment 30

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Highlight pseudoclass and attribute texts in the selector text of the rule view [2.0]

Removed the highlight of the non-pseudoclass locks for now. We will add in highlight colours for attributes and pseudoclasses in a follow up when shorlander is back.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f7a03693c031
https://hg.mozilla.org/integration/fx-team/rev/34740941d7fd

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f7a03693c031
https://hg.mozilla.org/mozilla-central/rev/34740941d7fd

Comment 33

[Stephen Horlander [:shorlander]]

(In reply to Gabriel Luong [:gl] from comment #30)
> Created attachment 8633587 [details] [diff] [review]
> Part 2: Highlight pseudoclass and attribute texts in the selector text of
> the rule view [2.0]
> 
> Removed the highlight of the non-pseudoclass locks for now. We will add in
> highlight colours for attributes and pseudoclasses in a follow up when
> shorlander is back.

Is there a follow-up for those?

Comment 34

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Stephen Horlander [:shorlander] from comment #33)
> (In reply to Gabriel Luong [:gl] from comment #30)
> > Created attachment 8633587 [details] [diff] [review]
> > Part 2: Highlight pseudoclass and attribute texts in the selector text of
> > the rule view [2.0]
> > 
> > Removed the highlight of the non-pseudoclass locks for now. We will add in
> > highlight colours for attributes and pseudoclasses in a follow up when
> > shorlander is back.
> 
> Is there a follow-up for those?

Yes, there is https://bugzilla.mozilla.org/show_bug.cgi?id=1183743