1149346 - Improve inspector selector search to include class/id results even without css prefix

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Brian Grinstead [:bgrins]]

Breaking this out of 835896 to simplify things.  This Bug is to land the changes to improve the existing selector search such that when searching 'd' on the following page:

<div></div>
<p id="d1"></p>
<p class="d2"></p>

The results will include div, #d1, and .d2.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: selector-search.patch

Rebased bug835896-better-selector-search v2.patch

Comment 2

[Brian Grinstead [:bgrins]]

Pushed current patch to try just to see where it is at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0a609991abc

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'

Comment 4

[Brian Grinstead [:bgrins]]

Patrick, this patch seems pretty much complete, even the try push is green.  And I believe it got an r+ from harth.  I see in https://bugzilla.mozilla.org/show_bug.cgi?id=835896#c44 you mention it had to be backed out because:

> When you type a class name without a . in style
> editor, the proper selector is suggested but the . is not inserted in the
> editor.

I'm not seeing this anymore with the patch applied running locally.  Not sure if you fixed it in another round or if it sorted itself out.  If you made any additional changes I'm happy to review them, otherwise can we just land this?

Comment 5

[Brian Grinstead [:bgrins]]

My guess is that this is going to have to be rebased on top of 873443, and that we will want to update browser_inspector_search-suggests-ids-and-classes.js to match the new test style

Comment 6

[Patrick Brosset <:pbro>]

Attachment: search.gif

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Patrick, this patch seems pretty much complete, even the try push is green. 
> And I believe it got an r+ from harth.  I see in
> https://bugzilla.mozilla.org/show_bug.cgi?id=835896#c44 you mention it had
> to be backed out because:
> 
> > When you type a class name without a . in style
> > editor, the proper selector is suggested but the . is not inserted in the
> > editor.
> 
> I'm not seeing this anymore with the patch applied running locally.  Not
> sure if you fixed it in another round or if it sorted itself out.  If you
> made any additional changes I'm happy to review them, otherwise can we just
> land this?
I'm still seeing this problem with the patch applied, see the attached animated gif (tested on about:home).
I believe this needs some changes in the client-side code.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> Created attachment 8591569 [details]
> search.gif
> 
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > Patrick, this patch seems pretty much complete, even the try push is green. 
> > And I believe it got an r+ from harth.  I see in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=835896#c44 you mention it had
> > to be backed out because:
> > 
> > > When you type a class name without a . in style
> > > editor, the proper selector is suggested but the . is not inserted in the
> > > editor.
> > 
> > I'm not seeing this anymore with the patch applied running locally.  Not
> > sure if you fixed it in another round or if it sorted itself out.  If you
> > made any additional changes I'm happy to review them, otherwise can we just
> > land this?
> I'm still seeing this problem with the patch applied, see the attached
> animated gif (tested on about:home).
> I believe this needs some changes in the client-side code.

Thanks for the gif - I am seeing this and now have a failing test case set up locally.  I'll take a look at this one and attach a follow up patch to be applied on top of yours that is already on the bug.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: autocomplete-sourceeditor-failing-test-case.patch

With this test change, we get:

TEST-UNEXPECTED-FAIL | browser/devtools/sourceeditor/test/browser_editor_autocomplete_events.js | Editor text has been updated - Got b2, expected #b2

Now just need to figure out how to fix it

Comment 9

[Brian Grinstead [:bgrins]]

Found another bug when testing this (Bug 1154371)

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: autocomplete-sourceeditor.patch

This seems to do the trick.  I've extended the test coverage to include cycling with the keyboard, since that's handled in a separate code path that wasn't covered.  Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5860a0bd9c8

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: selector-search.patch

Original patch written by Patrick and r+ed by harth, with updated expected results in browser_inspector_search-04.js (which didn't exist when this patch was written).  Also made two trivial changes to each of the test files - updated formatSuggestions with some additional parens so the correct count is logged out and changed the assertion order to be (actual, expected) instead of vice versa.

Comment 12

[Brian Grinstead [:bgrins]]

Updated try push with the fix for browser_inspector_search-04.js: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3eaafb5e669

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8592375 [details] [diff] [review]
autocomplete-sourceeditor.patch

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

I honestly have very little idea of what the cursor management code you added does, but you extracted the logic into its own function, it's not too complex, and you added a nice test for it, so I'm fine with it.

Comment 14

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/fd1fddc74fe8
remote:   https://hg.mozilla.org/integration/fx-team/rev/955b413d5bb3

Comment 15

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/fd1fddc74fe8
https://hg.mozilla.org/mozilla-central/rev/955b413d5bb3