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

VERIFIED FIXED in Firefox 51

Status

P1
normal
VERIFIED FIXED
2 years ago
2 months ago

People

(Reporter: magicp.jp, Assigned: steveck)

Tracking

Trunk
Firefox 51
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 verified)

Details

(Whiteboard: [reserve-html])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8781328 [details]
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.
(Reporter)

Updated

2 years ago
Blocks: 1265759
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox50: --- → unaffected
status-firefox51: --- → affected
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All

Updated

2 years ago
Blocks: 1263741
Flags: qe-verify?
Whiteboard: [devtools-html] [triage]

Comment 1

2 years ago
Anyone who works on this, it would be nice to add a test so we don't regress this.
No longer blocks: 1263741
Whiteboard: [devtools-html] [triage]

Comment 2

2 years ago
Whoops
Whiteboard: [devtools-html] [triage]

Updated

2 years ago
Blocks: 1263741
(Assignee)

Comment 3

2 years ago
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).
(Assignee)

Comment 4

2 years ago
Created attachment 8781506 [details] [diff] [review]
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?
Attachment #8781506 - Flags: feedback?(ntim.bugs)

Comment 5

2 years ago
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.
Attachment #8781506 - Flags: feedback?(ntim.bugs)
(Assignee)

Comment 6

2 years ago
(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
Flags: needinfo?(ntim.bugs)

Comment 7

2 years ago
Ah makes sense then, feel free to go ahead with comment 4
Flags: needinfo?(ntim.bugs)

Comment 8

2 years ago
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
Flags: needinfo?(schung)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
(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.
Flags: needinfo?(schung)
(Assignee)

Updated

2 years ago
Assignee: nobody → schung
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
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?
Attachment #8782271 - Flags: review?(ntim.bugs) → review+

Updated

2 years ago
Priority: -- → P3
Whiteboard: [devtools-html] [triage] → [reserve-html]
(Assignee)

Comment 13

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Flags: needinfo?(ntim.bugs)

Comment 15

2 years ago
(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.
Flags: needinfo?(ntim.bugs)

Updated

2 years ago
Keywords: checkin-needed

Updated

2 years ago
Attachment #8781506 - Attachment is obsolete: true
Needs rebasing.
Keywords: checkin-needed
Comment hidden (mozreview-request)
(Assignee)

Comment 18

2 years ago
Rebased
Keywords: checkin-needed

Comment 19

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/e5f3c2ed25f5
Don't hold search result after clearing inspector-searchbox by inspector-searchinput-clear. r=ntim
Keywords: checkin-needed

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e5f3c2ed25f5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

2 years ago
Iteration: --- → 51.2 - Aug 29
Flags: qe-verify? → qe-verify+
Priority: P3 → P1
QA Contact: cristian.comorasu
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.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.