Closed Bug 1225514 Opened 4 years ago Closed 4 years ago

"Search HTML" suggestions appear to be broken when I enter a selector

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox45+ verified)

VERIFIED FIXED
Firefox 45
Tracking Status
firefox45 + verified

People

(Reporter: arni2033, Assigned: pbro)

References

()

Details

(Keywords: regression, Whiteboard: [bugday-20160120])

Attachments

(2 files, 2 obsolete files)

>>> My Info:   Win7_64, Nightly 45, 32bit, ID 20151116030208, new profile <<<
STR:
1. Open this "data:" url:
>   data:text/html,<a b="cd"><a b="ce"><a b="cf">
2. Open devtools -> Inspector
3. Paste     a[b*='c'     in the field "Search HTML"   (you'll see suggestion "a["  )
4. Press Enter key

Result:       My carefully typed input was replaced with "a"
Expectations: The first <a> element should be selected
Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f00a24682d57e48a287e11bcfc72e562929fd007&tochange=961911623a6f2ec1d036c7b12a5117ebbeff45d8

Leaving unconfirmed, because, who knows, maybe there's already plan to fix that.
Workaround:
> 4. Press Esc key to hide suggestions, then Enter key.
I can confirm, thanks for the bug report
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
I also see weird behavior with `data:text/html,<div></div>` and then searching for `body > *`.  The suggestion is replaced with `body`
[Tracking Requested - why for this release]: This is a pretty obvious bug with a new relnote worthy feature (full text inspector search).  For input formed in certain ways, the search suggestion popup is broken.
The SelectorAutocompleter.prototype._onSearchKeypress function (in \devtools\client\inspector\inspector-search.js) does something when ENTER is pressed:

this.searchBox.value = this.searchPopup.selectedItem.label;

that sets the value of the search box. But it turns out that in the use case in comment 0, the label is 'a'. However there's another value that seems more correct: this.searchPopup.selectedItem.preLabel.
I'm going to investigate why we use label instead of preLabel and see if I can find a fix.
This part of inspector-search.js is really buggy. I found a couple of other bugs just looking at it for 5 minutes.

For instance, try data:text/html,<a class="test" id="id1">
and enter #id1.tes
Some of the regexp in the code do not expect ids and classes to have non letter characters.

Because this is a regression (works fine in 44), I'm tempted to propose a simple (2 lines) fix I found for this, and file a new bug to rework this entire piece of code.
Blocks: 1229773
Would something like this be enough for now? (so we land something before the merge date and avoid shipping with a regression)
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Attachment #8694728 - Flags: review?(bgrinstead)
Ah, last minute change, this also handles cases like 'body a[b=c]'
Attachment #8694728 - Attachment is obsolete: true
Attachment #8694728 - Flags: review?(bgrinstead)
Attachment #8694738 - Flags: review?(bgrinstead)
Comment on attachment 8694738 [details] [diff] [review]
Bug_1225514_-_Simple_fix_for_searching_with_attrib.diff

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

Thanks!  I'm happy with incremental fixes and later doing a bigger refactor.  As we discussed, this needs to still handle an unclosed attribute block and have a small regression test.  And then there's also the body > * case which could be handled in a separate patch in this bug if you'd prefer
Attachment #8694738 - Flags: review?(bgrinstead) → feedback+
Added a test for this, made the last ] character optional, and added an early bail-out for selectors ending with * (we hide the suggestion popup in aurora).

Tests pass locally, I will provide a try push shortly.
Attachment #8694738 - Attachment is obsolete: true
Attachment #8694820 - Flags: review?(bgrinstead)
Comment on attachment 8694820 [details] [diff] [review]
Bug_1225514_-_Simple_fix_for_searching_with_attrib.diff

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

This is great, thanks!

::: devtools/client/inspector/inspector-search.js
@@ +459,5 @@
>      let firstPart = "";
>  
> +    if (query.endsWith("*")) {
> +      // Hide the popup if we have some matching nodes and the query is not
> +      // ending with [.# >] which means that the selector is not at the

This comment is out of date - it shouldn't reference [.# >] anymore, just *.  And maybe also give an example query that we are early returning on here (body > *)
Attachment #8694820 - Flags: review?(bgrinstead) → review+
Thanks Brian, I'll update that comment.
Pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e704a4cfe65&selectedJob=14344310
https://hg.mozilla.org/mozilla-central/rev/92499b34eeff
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Has STR: --- → yes
Reproduced this bug with Firefox Nightly 45.0a1 (2015-11-17) (Build ID: 20151117030242) 
on Linux, 64 Bit with the instructions from comment 0.

This Bug is now verified as fixed on Latest Firefox Developer Edition 45.0a2 (2016-01-17)

Build ID: 20160117004011
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
QA Whiteboard: [testday-20160115]
I have reproduced this bug with Firefox Nightly 45.0a1 (Build ID: 20151117030242) on 
windows 8.1 64-bit with the instructions from comment 0.

Verified as fixed with latest Firefox Developer edition 45.0a2 (Build ID: 20160119004010)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0

As this bug is also verified on Linux(Comment 15), I am marking this as verified !
Status: RESOLVED → VERIFIED
Whiteboard: [bugday-20160120]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.