Closed
Bug 1225514
Opened 9 years ago
Closed 9 years ago
"Search HTML" suggestions appear to be broken when I enter a selector
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox45+ verified)
VERIFIED
FIXED
Firefox 45
People
(Reporter: arni2033, Assigned: pbro)
References
()
Details
(Keywords: regression, Whiteboard: [bugday-20160120])
Attachments
(2 files, 2 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
>>> 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.
Comment 2•9 years ago
|
||
I can confirm, thanks for the bug report
Comment 3•9 years ago
|
||
I also see weird behavior with `data:text/html,<div></div>` and then searching for `body > *`. The suggestion is replaced with `body`
Comment 4•9 years ago
|
||
[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.
tracking-firefox45:
--- → ?
Assignee | ||
Comment 5•9 years ago
|
||
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.
Assignee | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
Would something like this be enough for now? (so we land something before the merge date and avoid shipping with a regression)
Assignee | ||
Comment 8•9 years ago
|
||
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 9•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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+
Assignee | ||
Comment 12•9 years ago
|
||
Thanks Brian, I'll update that comment.
Pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e704a4cfe65&selectedJob=14344310
Comment 14•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Updated•9 years ago
|
Comment 15•9 years ago
|
||
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]
Comment 16•9 years ago
|
||
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 !
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•