Closed Bug 1149346 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(3 files, 2 obsolete files)

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.
Attached patch selector-search.patch (obsolete) — Splinter Review
Rebased bug835896-better-selector-search v2.patch
Whiteboard: [devedition-40]
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
Pushed current patch to try just to see where it is at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0a609991abc
Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
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?
Flags: needinfo?(pbrosset)
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
Depends on: 873443
Attached image 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.
Flags: needinfo?(pbrosset)
(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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
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
Found another bug when testing this (Bug 1154371)
See Also: → 1154371
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
Attachment #8592264 - Attachment is obsolete: true
Attachment #8592375 - Flags: review?(pbrosset)
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.
Attachment #8585773 - Attachment is obsolete: true
Attachment #8592574 - Flags: review+
Updated try push with the fix for browser_inspector_search-04.js: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3eaafb5e669
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.
Attachment #8592375 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
remote:   https://hg.mozilla.org/integration/fx-team/rev/fd1fddc74fe8
remote:   https://hg.mozilla.org/integration/fx-team/rev/955b413d5bb3
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [fixed-in-fx-team][devedition-40][difficulty=easy]
https://hg.mozilla.org/mozilla-central/rev/fd1fddc74fe8
https://hg.mozilla.org/mozilla-central/rev/955b413d5bb3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][devedition-40][difficulty=easy] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.