Closed Bug 595560 Opened 9 years ago Closed 9 years ago

Make "/" and Ctrl-f TabCandy search shortcuts, have Panorama shortcut act like return

Categories

(Firefox Graveyard :: Panorama, enhancement, P3)

enhancement

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 4.0b8

People

(Reporter: quodlibetor, Assigned: raymondlee)

References

Details

Attachments

(1 file, 7 obsolete files)

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b6pre) Gecko/20100910 Firefox/4.0b6pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b6pre) Gecko/20100910 Firefox/4.0b6pre

As far as I can tell there are no keyboard search shortcuts for TabCandy, and obviously there should be now that there is that cool search ui.

As it is, if you just start typing it will do the search, but I didn't discover that until I banged my keyboard in frustration. (Which was actually probably the only time in history where that has actually helped.)

Anyway, I think that the already-existing keyboard search shortcuts will be more discoverable for keyboard users.

Reproducible: Always
Assigning to Aza for design.
Assignee: nobody → aza
Priority: -- → P3
It makes sense to have command/ctrl-f and / also bring up the search interface. I'm a little hesitant about adding more than one way to do things, but parity with the rest of the system takes precedent.

Here are the three things we should do:

* Add command/ctrl-f and / as methods of bringing up the search interface
* Have command-e, when there are search results, act the same as hitting return
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Make "/" and Ctrl-f TabCandy search shortcuts → Make "/" and Ctrl-f TabCandy search shortcuts, have Panorama shortcut act like return
Blocks: 597043
Assignee: aza → raymond
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
Attachment #479680 - Flags: feedback?(ian)
Comment on attachment 479680 [details] [diff] [review]
v1

Passed try!
Attachment #479680 - Attachment is patch: true
Attachment #479680 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 479680 [details] [diff] [review]
v1

Rather than creating new key handlers for these existing commands, it would be more robust to hook into the existing key handlers. Otherwise we'll break on localizations of the app that have modified the command keys, and will also break in the future if someone changes key commands.

Command + e just uses our standard hiding mechanism, so you should put a check in there to see if search is up, and use the selected item if so. Note that I've done some work in this area in bug 595374, so you may or may not want to base your patch on it. 

I don't know where command + f and slash are handled, but you should find that spot and hook into it. 

I feel like the test could be more DRY; just something to think about.
Attachment #479680 - Flags: feedback?(ian) → feedback-
(In reply to comment #5)
> Comment on attachment 479680 [details] [diff] [review]
> v1
> 
> Rather than creating new key handlers for these existing commands, it would be
> more robust to hook into the existing key handlers. Otherwise we'll break on
> localizations of the app that have modified the command keys, and will also
> break in the future if someone changes key commands.
> 
> Command + e just uses our standard hiding mechanism, so you should put a check
> in there to see if search is up, and use the selected item if so. Note that
> I've done some work in this area in bug 595374, so you may or may not want to
> base your patch on it. 

Will do that.


> 
> I don't know where command + f and slash are handled, but you should find that
> spot and hook into it. 

The command + f is defined in "key_find" element but as you mentioned the localizations of app could modified the command keys.  I don't think we should hook that into it because we might end up with command + keyA for find feature and command + keyB for panorama search.  IMO, we should still use our own key handler for that but get the key from the properties (language string) file. 
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/help/content/help.xul#120


> 
> I feel like the test could be more DRY; just something to think about.

Yeah, will make it more DRY.
Attached patch v1 (obsolete) — Splinter Review
Attachment #479680 - Attachment is obsolete: true
Attachment #480561 - Flags: feedback?(ian)
Comment on attachment 480561 [details] [diff] [review]
v1

(In reply to comment #6)
> > I don't know where command + f and slash are handled, but you should find that
> > spot and hook into it. 
> 
> The command + f is defined in "key_find" element but as you mentioned the
> localizations of app could modified the command keys.  I don't think we should
> hook that into it because we might end up with command + keyA for find feature
> and command + keyB for panorama search.  IMO, we should still use our own key
> handler for that but get the key from the properties (language string) file. 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/help/content/help.xul#120

I'm not sure I understand what you mean. Isn't this why we should be hooking into their routine? If their key combo changes, we want it to change for us too. 

At any rate, loading their key combo instead of hooking into their routine is probably fine. You don't seem to be doing that in this patch, however; VK_SLASH is used directly, and F is loaded from tabview-specific strings. 

Also, speaking of slash, have you checked to see what happens if you type a slash into one of our text fields (a group title or the search box)? We need to make sure that that works normally and doesn't bring up the search box. 

+function getSearchTabMacher() {
+  return new TabMatcher(iQ("#searchbox").val());
+}

Thanks for adding this and isSearchEnabled; even though they are simple routines, they make the calling code cleaner and clearer. 

One thing on naming: starting this routine name with "get" is confusing, as it makes it seem like you're just accessing an existing object rather than creating a new one. I'd recommend starting it with "create" or "getNew". 

+  // press \
+  EventUtils.synthesizeKey("VK_SLASH", { type: "keydown" }, contentWindow);

That comment shows a backslash, when it should be a slash (or probably just spelled out). 

Thanks for making the test more DRY!
Attachment #480561 - Flags: feedback?(ian) → feedback-
Attached patch v1 (obsolete) — Splinter Review
* Handled the cmd+F and / as we discussed in IRC
* / works fine in textfields in the tabview UI.
Attachment #480561 - Attachment is obsolete: true
Attachment #480837 - Flags: feedback?(ian)
Attachment #480837 - Attachment is patch: true
Attachment #480837 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 480837 [details] [diff] [review]
v1

Looking good. A couple questions: 

I see you're using a literal VK_SLASH; is that key not listed somewhere we can grab?

+             oncommand="if (TabView.isVisible()) TabView.enableSearch(event); else gFindBar.onFindCommand();"

Is "event" a valid variable in this context?
Attachment #480837 - Flags: feedback?(ian) → feedback+
(In reply to comment #10)
> Comment on attachment 480837 [details] [diff] [review]
> v1
> 
> Looking good. A couple questions: 
> 
> I see you're using a literal VK_SLASH; is that key not listed somewhere we can
> grab?
> 

The key isn't listed elsewhere as discussed on IRC.

> +             oncommand="if (TabView.isVisible()) TabView.enableSearch(event);
> else gFindBar.onFindCommand();"
> 
> Is "event" a valid variable in this context?

Yes, it is an valid variable.
Attachment #480837 - Flags: review?(dolske)
Comment on attachment 480837 [details] [diff] [review]
v1

>+function createSearchTabMacher() {
>+  return new TabMatcher(iQ("#searchbox").val());
>+}

A little meh on that, but also meh on my meh. :)
Attachment #480837 - Flags: review?(dolske) → review+
blocking2.0: --- → ?
Attached patch v1 (obsolete) — Splinter Review
r=dolske, sent it to try again to double check it.
Attachment #480837 - Attachment is obsolete: true
Attachment #485652 - Flags: approval2.0?
blocking2.0: ? → ---
Attached patch Patch for check-in (obsolete) — Splinter Review
Minor change to fix a test. Sent to try
Attachment #485652 - Attachment is obsolete: true
Attachment #485687 - Flags: approval2.0?
Attachment #485652 - Flags: approval2.0?
(In reply to comment #14)
> Created attachment 485687 [details] [diff] [review]
> Patch for check-in
> 
> Minor change to fix a test. Sent to try

Passed try
Blocks: 605091
Comment on attachment 485687 [details] [diff] [review]
Patch for check-in

a=beltzner, best part, no new strings needed!
Attachment #485687 - Flags: approval2.0? → approval2.0+
Attached file Patch for check-in (obsolete) —
Attachment #485687 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
Attached patch Patch for check-in (obsolete) — Splinter Review
Attachment #492699 - Attachment is obsolete: true
Attachment #492922 - Attachment is obsolete: true
Sent it to try and passed with intermittent orange.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/04493b2a23d8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
Verified in recent nightly minefield build
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.