searching attribute values through CSS selectors override the search terms

RESOLVED FIXED in Firefox 49

Status

P2
normal
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: karlcow, Assigned: nchevobbe)

Tracking

44 Branch
Firefox 49

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
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).
Filter on CLIMBING SHOES
Priority: -- → P2
Blocks: 1264624
(Assignee)

Updated

3 years ago
Assignee: nobody → chevobbe.nicolas

Updated

3 years ago
Blocks: 1229773
(Assignee)

Comment 3

3 years ago
Should we show the suggestion popup when searching on attribute ? And if so, how ?
Flags: needinfo?(pbrosset)
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].
Flags: needinfo?(pbrosset)
(Assignee)

Comment 5

3 years ago
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/
Attachment #8746640 - Flags: review?(pbrosset)
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.
Attachment #8746640 - Flags: review?(pbrosset)
(Assignee)

Comment 7

3 years ago
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
(Assignee)

Comment 8

3 years ago
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/
Attachment #8746640 - Flags: review?(pbrosset)
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.
Attachment #8746640 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 10

3 years ago
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/
(Assignee)

Comment 11

3 years ago
TRY (https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a80e3921219) is over and everything seems fine
Keywords: checkin-needed

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bdb07aa13219
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
No longer blocks: 1264624

Updated

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