1263104 - Ctrl+F in Storage Inspector should open search

Status: VERIFIED FIXED

(DevTools :: Storage Inspector, defect, P3)

Description

[Jens]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160315153207

Steps to reproduce:

1. Open devtools
2. Go to Inspector or Storage (possibly other panels too)
3. Press Ctrl+f (on Windows in my case)


Actual results:

Native Firefox search box opens


Expected results:

The panel's search box should open, since I am in this context. The console, debugger and style editor do it this way.

For the inspector there are actually two search boxes: "Search HTML" and "Filter styles" - depending on focus or mouse position either of one should open. Default should be "Search HTML" if in doubt.

Maybe split this up in two bugs depending on how you work on this teamwise: One for Inspector, one for Storage.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Helen, any opinions here?  I think was some ongoing discussion about unifying filtering already...

Comment 2

[Helen V. Holmes (:helenvholmes) (:✨)]

This sounds pretty logical to me. Do we potentially want to split this bug to address the Inspector and the Storage panel respectively?

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Okay, I'll split it up.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Actually, it seems to already work for me in the Inspector.  With a markup node focused, I pressed Cmd-F (maybe we need to test Windows to be sure...) and it focused the "Search HTML" box.

Jens, can you check the inspector behavior in Nightly 48 on your side?

Comment 5

[Brian Grinstead [:bgrins]]

ctrl+F should be working in the Inspector on Windows based on https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector.xul#28-38.  But if not, we should definitely fix that.  If it is, we can leave this bug open and move it to the storage inspector component.

Comment 6

[arni2033]

Wasn't Inspector fixed in bug 1260235?

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to arni2033 from comment #6)
> Wasn't Inspector fixed in bug 1260235?

Yeah that's it, thanks

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Looks like both of these see also bugs for the inspector were fixed in 48, while the reporter appears to using an earlier version if the bug settings / comment 0 are correct.  Anyway, hopefully we'll hear back from them conclusively. :)

Comment 9

[Jens]

Sure you hear back ;)

When I reported in Comment 0 I reassured the bug existed using Developer Edition (47). Whereas I reported using FF 45 (unfortunately, too many glitches in the Dev Edition to use it during development)...

Tested now again using nightly 48.0a1 (2016-04-12). My findings:

1. Inspector doesn't open native search anymore on Ctrl+f. Indeed, it is *very* nice now because it open either the left-side "Search HTML" or the right-side "Filter Styles" depending on where you are currently. Really good, well done!

2. Storage still open the native search on Ctrl+f and not the "Filter items". BTW, shouldn't this be "Filter Items" instead like it's "Filter Styles" in the Inspector - you might know better, I am no native speaker (but it should be consistant). I am on Windows, in case this is important.

(btw, here on windows when running "normal" FF 45 I cannot start Nightly (it just launches FF 45 again) making such tests more complicated. Nightly is only startable if I previously close the FF 45. Would be nice if Nightly behaved like Dev Edition here)

Comment 10

[Brian Grinstead [:bgrins]]

Thanks for the response.  Moving this into the storage inspector component

Comment 11

[Jens]

Attachment: debugger_search_input_ff48_nightly.png

@bgrins: Just started Nightly and saw that in the Debugger it now says "Search scripts (Ctrl+P)", see screenshot:

1. Ctrl+F still works, so I don't understand why its handled inconsistent here. It should be Ctrl+F everywhere
2. Nit: Again not capitilized (Search Scripts).

So, should be changed to: "Search Scripts (Ctrl+F)". Surfacing this shortcut in each search box would be good though.

Should I open another ticket for this? Since the discussion started about inconsistencies here, I posted here first.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Jens from comment #11)
> Created attachment 8741779 [details]
> debugger_search_input_ff48_nightly.png
> 
> @bgrins: Just started Nightly and saw that in the Debugger it now says
> "Search scripts (Ctrl+P)", see screenshot:
> 
> 1. Ctrl+F still works, so I don't understand why its handled inconsistent
> here. It should be Ctrl+F everywhere
> 2. Nit: Again not capitilized (Search Scripts).
> 
> So, should be changed to: "Search Scripts (Ctrl+F)". Surfacing this shortcut
> in each search box would be good though.
> 
> Should I open another ticket for this? Since the discussion started about
> inconsistencies here, I posted here first.

Yes, please file another bug for that in the Debugger component.  I'm not sure why it's showing ctrl+p either - they seem to have slightly different behavior but ctrl+f seems like the one that people would want.

Comment 13

[Luke Chang [:lchang] (inactive)]

I'd like to try on this.

Comment 14

[Luke Chang [:lchang] (inactive)]

Attachment: WIP.patch

I'll be on the PTO from today until 6/10 so may not update the bug status promptly. Attached is my early WIP patch, just in case.

Comment 15

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8759055 [details] [diff] [review]
WIP.patch

Hi Michael,

Would you mind giving me some feedback about this patch? It was basically modified from "inspector-panel.js". Besides, should I add some tests for that? Thanks.

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8759055 [details] [diff] [review]
WIP.patch

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

(In reply to Luke Chang [:lchang] from comment #15)
> Comment on attachment 8759055 [details] [diff] [review]
> WIP.patch
> 
> Hi Michael,
> 
> Would you mind giving me some feedback about this patch? It was basically
> modified from "inspector-panel.js". Besides, should I add some tests for
> that? Thanks.

Your patch works great when I manually test it so that is great.

We don't have tests for this in other places because testing focus in tests often causes them to intermittently fail due to async issues.

As long as this does what we want we are happy with it.

That said, you will need to put it through try before I r+ it.

If you don't know what try is or don't have permission to access it then feel free to needinfo me and I will do it for you.

Comment 17

[Luke Chang [:lchang] (inactive)]

Hi Michael,

Thanks for your feedback. I'm trying to write mochitests and hope it works.

BTW, my account of try server has been disabled due to inactivity. I'm requesting to re-activate it in bug 1282404 so it may take some time to run it on try server.

Comment 18

[Luke Chang [:lchang] (inactive)]

Thank Fischer for helping me to trigger try server (because my account is still disabled):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e8e55aabf99&selectedJob=23010319


Hi Michael,

I saw many fails in the report but don't know how to interpret them. It seems they're not caused by my patch. Should I run it again?

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9b8220dbc6e

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Your patch was applied on top of a failing changeset.

I have applied the patch on top of a known good changeset and pushed to try again (comment 19).

Hope this helps.

Comment 21

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1263104 - Ctrl+F in Storage Inspector should open search

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

Comment 22

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8766690 [details]
Bug 1263104 - Ctrl+F in Storage Inspector should open search

Hi Michael,

According to comment 19, it looks better since there are only two fails which seem not to be caused by my patch.

In addition, I've added a mochitest based on the inspector's one from "browser_inspector_search_keyboard_trap.js".

Could you please review this patch? Thanks.

Comment 23

[Luke Chang [:lchang] (inactive)]

Run try server again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a5c71e77290

Comment 24

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8766690 [details]
Bug 1263104 - Ctrl+F in Storage Inspector should open search

https://reviewboard.mozilla.org/r/61500/#review58788

Comment 25

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Awesome work Luke... let us know if you want another bug to work on.

Comment 26

[Luke Chang [:lchang] (inactive)]

Michael, Thanks for your review and glad I could help.

Comment 27

[Carsten Book [:Tomcat]]

has problems to apply

applying 8311a3dd0d97
patching file devtools/client/storage/test/browser.ini
Hunk #1 FAILED at 29
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/storage/test/browser.ini.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 28

[Luke Chang [:lchang] (inactive)]

Hi Carsten,

May I know which repository you are going to apply the patch? I tried to merge the patch to inbound and fx-team locally and didn't encounter exceptions.

Comment 29

[Carsten Book [:Tomcat]]

(In reply to Luke Chang [:lchang] from comment #28)
> Hi Carsten,
> 
> May I know which repository you are going to apply the patch? I tried to
> merge the patch to inbound and fx-team locally and didn't encounter
> exceptions.

Hi Luke, i tried to merge against fx-team when it failed, there were also a changes like bug 1283800 that might cause this ?

Comment 30

[Luke Chang [:lchang] (inactive)]

Attachment: patch_for_fx-team.patch

I tried to adapt my patch to fx-team. Could you try this again? Thanks.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/2a6a9b88b5ba
Ctrl+F in Storage Inspector should open search. r=mratcliffe

Comment 32

[Carsten Book [:Tomcat]]

worked perfect now, thanks Luke!

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a6a9b88b5ba

Comment 34

[Tanvir Rahman]

I have reproduced this bug with nightly 48.0a1 (2016-04-08) on Windows 10, 64 bit!

The Bug's fix is now verified on latest nightly 50.0a1.

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

[bugday-20160713]

Comment 35

[Rezaul Huque Nayeem]

Reproduced the bug in firefox nightly 48.0a1 (2016-04-08) with ubuntu 16.04 (64 bit)

Verified as fixed with latest firefox nightly 50.0a1 (Build ID: 20160715063552)
Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0