1295390 - Don't hold search result after clearing inspector-searchbox by inspector-searchinput-clear

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[magicp]

Attachment: do-not-hold-search-result-after-clearing-inspector-search-box.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160815030201

Steps to reproduce:

1. Start Nightly
2. Go to about:home
3. Open DevTools > Inspector
4. Enter "div" into inspector-searchbox  => The search result will be "1 of xx"
5. Click inspector-searchinput-clear (x) icon
6. Check inspector-searchbox and inspector-searchlabel



Actual results:

inspector-searchbox is cleared, but inspector-searchlabel is still holding the search result.

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=c9bbdb627b7804fee47aa6a6708647e6e589d09c&tochange=3269dd1a824d1b42cb021d1fb6858885179940b0


Expected results:

After clicking inspector-searchinput-clear, inspector-searchlabel should be reset.

Comment 1

[Tim Nguyen :ntim]

Anyone who works on this, it would be nice to add a test so we don't regress this.

Comment 2

[Tim Nguyen :ntim]

Whoops

Comment 3

[Steve Chung [:steveck]]

I can take a look, but I would need more time for the testing part(it seems need to be part of browser_inspector_search test set).

Comment 4

[Steve Chung [:steveck]]

Attachment: bug-1295390.patch

Hi Tim, I add the clear event emitter (and style removal) for the clear button event listener,and I think I might need to add the test in browser_inspector_search-06. Wdyt?

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8781506 [details] [diff] [review]
bug-1295390.patch

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

We're not doing anything asynchronous when clearing the input, so there's no need for an event.

Normally you should just be able to click the clear button in the test, then directly test if everything is properly reset without waiting for an event.

Comment 6

[Steve Chung [:steveck]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #5)
> Comment on attachment 8781506 [details] [diff] [review]
> bug-1295390.patch
> 
> Review of attachment 8781506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We're not doing anything asynchronous when clearing the input, so there's no
> need for an event.
> 
It seems like we still leverage the events for the label update[1]. Since it's only for label, maybe we could simply set the label element for searchbox instance and maintain the label inside the searchbox instead of inspector panel. I'm fine with both solutions, and please let me know if you would prefer to handle all the related UI updating in the same time rather than doing it asynchronously.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector-panel.js#360

Comment 7

[Tim Nguyen :ntim]

Ah makes sense then, feel free to go ahead with comment 4

Comment 8

[[e-mail]]

The goal is to reuse sidebar searchbox oneday so :gl said its better to keep the label out of the searchbox

I also added a clear button related test in bug 1295081

Comment 9

[Steve Chung [:steveck]]

Attachment: Bug 1295390 - Don't hold search result after clearing inspector-searchbox by inspector-searchinput-clear.

Review commit: https://reviewboard.mozilla.org/r/72470/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72470/

Comment 10

[Steve Chung [:steveck]]

(In reply to Fred Lin [:gasolin] from comment #8)
> The goal is to reuse sidebar searchbox oneday so :gl said its better to keep
> the label out of the searchbox

Thanks for the explanation!
 
> I also added a clear button related test in bug 1295081

I added another test file for label as well to avoid the possible conflicts or dependency issue.

Comment 11

[Steve Chung [:steveck]]

Comment on attachment 8782271 [details]
Bug 1295390 - Don't hold search result after clearing inspector-searchbox by inspector-searchinput-clear.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72470/diff/1-2/

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8782271 [details]
Bug 1295390 - Don't hold search result after clearing inspector-searchbox by inspector-searchinput-clear.

https://reviewboard.mozilla.org/r/72470/#review70910

Thanks!
r=me with those addressed.

::: devtools/client/inspector/test/browser.ini:144
(Diff revision 2)
>  [browser_inspector_search-08.js]
>  [browser_inspector_search_keyboard_trap.js]
>  [browser_inspector_search-reserved.js]
>  [browser_inspector_search-selection.js]
>  [browser_inspector_search-sidebar.js]
> +[browser_inspector_search_label.js]

nit: use browser_inspector_search-label.js instead of _label.js for more consistency with other test names.

::: devtools/client/inspector/test/browser_inspector_search_label.js:33
(Diff revision 2)
> +  // Catch-all event for remaining server requests when searching for the new
> +  // node.
> +  yield inspector.once("inspector-updated");
> +});
> +
> +function* synthesizeKeys(keys, inspector) {

So, looks like this function is already defined in head.js, with a different 2nd parameter (takes inspector.panelWin instead of just inspector). 

Can you remove this definition and the one in browser_inspector_search-06.js and adjust the second parameter in those 2 files to use inspector.panelWin?

Comment 13

[Steve Chung [:steveck]]

Comment on attachment 8782271 [details]
Bug 1295390 - Don't hold search result after clearing inspector-searchbox by inspector-searchinput-clear.

https://reviewboard.mozilla.org/r/72470/#review70910

> So, looks like this function is already defined in head.js, with a different 2nd parameter (takes inspector.panelWin instead of just inspector). 
> 
> Can you remove this definition and the one in browser_inspector_search-06.js and adjust the second parameter in those 2 files to use inspector.panelWin?

There's still slightly different between those 2 synthesizeKeys function: The general one in head.js handles the string and it will split into chars for EventUtils.synthesizeKey, but the one in  browser_inspector_search-06.js handles array, which could deal VK_RETURN key properly. It also wait for lastQuery result, so maybe it's to avoid any possible intermittent error issue. Anyway I remove the function and reuse the original synthesizeKeys/EventUtils.synthesizeKey for fitting the search requirement, but I still kept the one in search-06.js. wdyt?

Comment 14

[Steve Chung [:steveck]]

Comment on attachment 8782271 [details]
Bug 1295390 - Don't hold search result after clearing inspector-searchbox by inspector-searchinput-clear.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72470/diff/2-3/

Comment 15

[Tim Nguyen :ntim]

(In reply to Steve Chung [:steveck] from comment #13)
> Comment on attachment 8782271 [details]
> Bug 1295390 - Don't hold search result after clearing inspector-searchbox by
> inspector-searchinput-clear.
> 
> https://reviewboard.mozilla.org/r/72470/#review70910
> 
> > So, looks like this function is already defined in head.js, with a different 2nd parameter (takes inspector.panelWin instead of just inspector). 
> > 
> > Can you remove this definition and the one in browser_inspector_search-06.js and adjust the second parameter in those 2 files to use inspector.panelWin?
> 
> There's still slightly different between those 2 synthesizeKeys function:
> The general one in head.js handles the string and it will split into chars
> for EventUtils.synthesizeKey, but the one in  browser_inspector_search-06.js
> handles array, which could deal VK_RETURN key properly. It also wait for
> lastQuery result, so maybe it's to avoid any possible intermittent error
> issue. Anyway I remove the function and reuse the original
> synthesizeKeys/EventUtils.synthesizeKey for fitting the search requirement,
> but I still kept the one in search-06.js. wdyt?
Sounds fine, ideally we could adjust the one in head.js to make it match the needs of search-06.js, but the change is out of scope of this bug.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 17

[Steve Chung [:steveck]]

Comment on attachment 8782271 [details]
Bug 1295390 - Don't hold search result after clearing inspector-searchbox by inspector-searchinput-clear.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72470/diff/3-4/

Comment 18

[Steve Chung [:steveck]]

Rebased

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/e5f3c2ed25f5
Don't hold search result after clearing inspector-searchbox by inspector-searchinput-clear. r=ntim

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5f3c2ed25f5

Comment 21

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this bug using Fx 51.0a1, build ID:  20160815030201, on Windows 10 x64.
I can confirm the bug is fixed, I verified using Fx 51.0a1 build ID: 20160824030337 , on Windows 10 x64, Ubuntu 14.04 LTS and Mac OS X 10.9.5.