1245365 - searching attribute values through CSS selectors override the search terms

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Karl Dubost💡 :karlcow]

Step to reproduce:

1. Open the inspector on a page with multiple links, script, etc.
2. In the search box, enter
   [src^=http]
3. a drop down appears with the list of matches

Expected: 
Being able to navigate all the node matching my query and having the query as-is in the search box.

Actual:
Matches an element then navigate through this element on return if it matches or not the pattern. For example, if it matches script in the dropdown, it will continue to navigate through other script elements even if they do not have the `src="http…` attribute value.

Comment 1

[Patrick Brosset <:pbro>]

This is really bad. The suggestion popup is also all messed up.
For instance:

- open mozilla.org
- open inspector
- enter [src^=http] in the search box
- the suggestion popup appears and contains one element: [src^=h which is wrong
- selecting this element from the suggestion list replaces the input field value with script
- even if you don't select the element from the list, pressing <enter> will have the same effect.

The search box doesn't support attribute selectors well at all.

Confirmed on both nightly (47) and aurora (46).

Comment 2

[Patrick Brosset <:pbro>]

Filter on CLIMBING SHOES

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Should we show the suggestion popup when searching on attribute ? And if so, how ?

Comment 4

[Patrick Brosset <:pbro>]

The suggestion popup is especially useful for tags, ids and classes (and in fact, I believe it only works with those). So we should not have a popup for things like [src^=http].

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Attachment: MozReview Request: Bug 1245365 - Fix markup view search with attribute selector. r=pbro

Add an ATTRIBUTE search state to better handle attribute's search.
Edit test to make sure we handle it right

Review commit: https://reviewboard.mozilla.org/r/49509/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49509/

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8746640 [details]
MozReview Request: Bug 1245365 - Fix markup view search with attribute selector. r=pbro

https://reviewboard.mozilla.org/r/49509/#review46399

Definitely a great start. Thanks for doing this.
I have found a few things though, please take a look below.

::: devtools/client/inspector/inspector-search.js:274
(Diff revision 1)
> +            }
> +          }
> +          break;
> +
> +        case this.States.ATTRIBUTE:
> +          if (subQuery.match(/[\[].*[\]]/) != null) {

The `.*` part will catch all characters including `]` which it shouldn't, right? We want everything except the closing bracket.
Should this be something like `/\[[^\]]+\]/` instead?

::: devtools/client/inspector/inspector-search.js:443
(Diff revision 1)
>          value = query.slice(0, -1 * lastPart.length + 1) + value;
>        } else if (query.match(/[a-zA-Z][#\.][^#\.\s+>]*$/)) {
>          // for cases like 'div.class' or '#foo.bar' and likewise
>          let lastPart = query.match(/[a-zA-Z][#\.][^#\.\s+>]*$/)[0];
>          value = query.slice(0, -1 * lastPart.length + 1) + value;
> -      } else if (query.match(/[a-zA-Z]\[[^\]]*\]?$/)) {
> +      } else if (query.match(/[a-zA-Z]*\[[^\]]*\]\./)) {

I think this misses some cases.
For instances, it works fine in cases like `[id].` (finds all nodes that have an id and suggests classnames), but doesn't for `[class]#`.

Which, btw, will require a new test case too.

::: devtools/client/inspector/inspector-search.js:502
(Diff revision 1)
>        // Hide the popup if the query ends with * because we don't want to
>        // suggest all nodes.

This comment needs updating. We also don't want to show the popup when searching for attributes because this can possibly result in a lot of less useful results too.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

https://reviewboard.mozilla.org/r/49509/#review46399

> The `.*` part will catch all characters including `]` which it shouldn't, right? We want everything except the closing bracket.
> Should this be something like `/\[[^\]]+\]/` instead?

You're right

> I think this misses some cases.
> For instances, it works fine in cases like `[id].` (finds all nodes that have an id and suggests classnames), but doesn't for `[class]#`.
> 
> Which, btw, will require a new test case too.

oh yes, I missed that. BTW, we could have [id]div too

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8746640 [details]
MozReview Request: Bug 1245365 - Fix markup view search with attribute selector. r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49509/diff/1-2/

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8746640 [details]
MozReview Request: Bug 1245365 - Fix markup view search with attribute selector. r=pbro

https://reviewboard.mozilla.org/r/49509/#review46677

Looks good to me now.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8746640 [details]
MozReview Request: Bug 1245365 - Fix markup view search with attribute selector. r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49509/diff/2-3/

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

TRY (https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a80e3921219) is over and everything seems fine

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bdb07aa13219

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bdb07aa13219