Closed Bug 1195038 Opened 9 years ago Closed 9 years ago

In about:home, make accel+f focus the search box instead of invoking the find bar

Categories

(Firefox :: Keyboard Navigation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: steffen.wilberg, Assigned: steffen.wilberg)

References

Details

Attachments

(1 file)

In about:home, accel+f should focus the search box instead of invoking the find bar.
Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. r?mak
Attachment #8648424 - Flags: review?(mak77)
https://reviewboard.mozilla.org/r/16203/#review14475

::: browser/base/content/abouthome/aboutHome.xhtml:46
(Diff revision 1)
> +               dir="auto" data-findkey="&find.commandkey;"/>

I stored the localized string in a HTML data attribute, so that I can retrieve it from non-privileged javascript.

Is there a better/recommended way to do this?
I'm currently on vacation, please ask for review to :adw or :gijs if you want a quicker reply.
Comment on attachment 8648424 [details]
MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.

Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. r?mak
Attachment #8648424 - Flags: review?(mak77) → review?(gijskruitbosch+bugs)
Can you elaborate on why we should do this?  I mean, Accel+f is associated with
"find in page" - why should we dilute that association by making it do something
completely different (search the web) on about:home?  Wouldn't Accel+k be a better
choice for what you want to do?

FWIW, Chrome has the same behavior for Accel+f that we do in its corresponding
"new tab" page that has a Google search box.  It invokes find-in-page so that
you can search the titles in the thumbnails presented there.  I find this to be
a useful feature.
One argument is indeed consistency: We use accel+f to focus the search/filter box in preferences (cookie viewer; passwords manager; I just fixed Applications; bug 1192540 fixes the regression in about:config; bug 661831 introduces that to about:permissions) and the Add-ons manager (to be fixed by bug 1195060). You might argue that most of those operate on the same page though (with the exception being the Add-ons manager, which searches on addons.mozilla.org), so this argument is rather weak.

A stronger argument is usefulness: On about:home, there is nothing to "find" on the page itself: You can't search for the buttons on the bottom and hit enter to activate them, even after closing the find bar. So the only useful action is to search on the web.

Likewise, on about:newtab, after finding a tile you need to press Esc to close the findbar to be able to activate the tile by pressing enter. I'd consider this a rather rare usecase because of that.

But I actually like the idea of stealing accel+k to focus the search box of about:home and about:newtab instead of focussing the search bar, because these 3 searches act and look exactly the same nowadays, with the benefit that the text is larger on about:home and about:newtab.
Comment on attachment 8648424 [details]
MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.

https://reviewboard.mozilla.org/r/16205/#review14493

::: browser/base/content/abouthome/aboutHome.js:24
(Diff revision 1)
> +var searchText, findKey;

Nit: let

::: browser/base/content/abouthome/aboutHome.js:59
(Diff revision 1)
> +window.addEventListener("keypress", (ev) => {

Nit: you don't need braces around (ev) here.

::: browser/base/content/abouthome/aboutHome.js:61
(Diff revision 1)
> +  if (ev.getModifierState("Accel") && ev.key == findKey) {

This doesn't check other modifier states, which means if other things take accel-shift-<same-shortcut-key> or accel-opt/alt-<same-shortcut-key>, this will steal those, too, which it shouldn't do.
Attachment #8648424 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8648424 [details]
MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.

I'd want UX to chime in here before taking this regardless. I am not entirely convinced it is a good idea.
Attachment #8648424 - Flags: ui-review?(philipp)
Comment on attachment 8648424 [details]
MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.

Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.
Attachment #8648424 - Attachment description: MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. r?mak → MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.
Attachment #8648424 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8648424 [details]
MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.

https://reviewboard.mozilla.org/r/16205/#review15279

With the below addressed, r=me, but you still need a ui-review.

::: browser/base/content/abouthome/aboutHome.js:61
(Diff revision 2)
> +  if (ev.getModifierState("Accel") && !ev.shiftKey && !ev.altKey

This isn't checking for ctrl on osx or meta on windows/linux.
Attachment #8648424 - Flags: review?(gijskruitbosch+bugs) → review+
https://reviewboard.mozilla.org/r/16205/#review15279

> This isn't checking for ctrl on osx or meta on windows/linux.

OK, I can fix it like this:
  let modifiers = ev.ctrlKey + ev.altKey + ev.shiftKey + ev.metaKey;
  if (ev.getModifierState("Accel") && modifiers == 1 && ev.key == findKey) {
(In reply to Steffen Wilberg from comment #11)
> https://reviewboard.mozilla.org/r/16205/#review15279
> 
> > This isn't checking for ctrl on osx or meta on windows/linux.
> 
> OK, I can fix it like this:
>   let modifiers = ev.ctrlKey + ev.altKey + ev.shiftKey + ev.metaKey;
>   if (ev.getModifierState("Accel") && modifiers == 1 && ev.key == findKey) {

WFM!
Comment on attachment 8648424 [details]
MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.

WFM too!
Attachment #8648424 - Flags: ui-review?(philipp) → ui-review+
Comment on attachment 8648424 [details]
MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.

Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.
Seems like we can land this? Steffen?
Flags: needinfo?(steffen.wilberg)
Comment on attachment 8648424 [details]
MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.

I added a test, please have a look.
Flags: needinfo?(steffen.wilberg)
Attachment #8648424 - Flags: review+ → review?(gijskruitbosch+bugs)
Comment on attachment 8648424 [details]
MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.

https://reviewboard.mozilla.org/r/16205/#review16537

LGTM but for the nit below.

::: browser/base/content/test/general/browser_aboutHome.js:436
(Diff revision 3)
> +    CustomizableUI.reset();

You don't need this line - we need it in the test you copy-pasted because setup() moves the searchbar - but you don't do that.
Attachment #8648424 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/integration/fx-team/rev/e1ff9d517b133e5f0022a2dee70e88c5c9573ad7
Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. r=gijs, ui-review=philipp
https://hg.mozilla.org/mozilla-central/rev/e1ff9d517b13
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Well, it just doesn't work. Ctrl+F still triggers findbar on about:home. Should this be reopened?
> My info:   Win7_64, Nightly 44, 32bit, ID 20150922030204, new profile, safe mode
> video:     https://dl.dropboxusercontent.com/s/tgr3vsoup6nwc6a/video%20-%20bug%201195038%20-%20just%20doesn%27t%20work.webm?dl=0
(In the end of video I just showed inconsist behavior - "Find" button isn't disabled on homepage)
Flags: needinfo?(steffen.wilberg)
(In reply to arni2033 from comment #21)
> Well, it just doesn't work. Ctrl+F still triggers findbar on about:home.
> Should this be reopened?

No, see bug 1204836.

> > My info:   Win7_64, Nightly 44, 32bit, ID 20150922030204, new profile, safe mode
> > video:     https://dl.dropboxusercontent.com/s/tgr3vsoup6nwc6a/video%20-%20bug%201195038%20-%20just%20doesn%27t%20work.webm?dl=0
> (In the end of video I just showed inconsist behavior - "Find" button isn't
> disabled on homepage)

FWIW, I can reproduce this if (and only if) focus is not inside the content page, which is expected given the patch here.
(In reply to arni2033 from comment #21)
> Well, it just doesn't work. Ctrl+F still triggers findbar on about:home.
> Should this be reopened?
> > My info:   Win7_64, Nightly 44, 32bit, ID 20150922030204, new profile, safe mode
> > video:     https://dl.dropboxusercontent.com/s/tgr3vsoup6nwc6a/video%20-%20bug%201195038%20-%20just%20doesn%27t%20work.webm?dl=0
> (In the end of video I just showed inconsist behavior - "Find" button isn't
> disabled on homepage)

FWIW there's something fishy going on in there as well, Ctrl+F isn't supposed to close the Findbar (unless you have an add-on like FindBar Tweak to enable that behavior).
(In reply to :Gijs Kruitbosch from comment #22)
> FWIW, I can reproduce this if (and only if) focus is not inside the content page, which is expected given the patch here.
On the following video it's clear that focus is inside the content. Also, I use Esc to close findbar
> video 2:   https://dl.dropboxusercontent.com/s/ezbm5buonvrnuh9/video%202%20-%20bug%201195038%20-%20just%20doesn%27t%20work.webm?dl=0
Actually, it's good that this patch will be canceled, because, well, it doesn't work.
Correction: I figured out that it works, but only on english keyboard layout. So this is basically one of the countless cases in firefox when shortcuts don't work in different locales. That's strange design
I personally think that there's no point in enhancements for 1 keyboard layout.
(In reply to arni2033 from comment #24)
> Also, I use Esc to close findbar
Obviously! I'm sorry about that. (What's happening with me today? I'm completely off my game...)

(In reply to arni2033 from comment #25)
> Correction: I figured out that it works, but only on english keyboard
> layout. So this is basically one of the countless cases in firefox when
> shortcuts don't work in different locales.
From your videos I assume you have a russian keyboard. I'm interested in this, do you know if there's some sort of tracking bug for inconsistencies in keyboard behavior like this? (Or maybe Gijs would know?) I wonder what the cause would be: find.commandkey entity localized differently; modifiers != 1; or event.key not actually being the exact same "f" on those layouts for whatever reason.
(In reply to Luís Miguel [:quicksaver] from comment #26)
> From your videos I assume you have a russian keyboard. I'm interested in this, do you know if
> there's some sort of tracking bug for inconsistencies in keyboard behavior like this?
I only know bug 626594, bug 591935. Also, here's my list of such shortcuts (it may be incomplete):
> Ctrl+Shift+Alt+I;  In devtools: Ctrl+[, Ctrl+];  In debugger: Ctrl+Alt+V (Ctrl+Alt+F was fixed)
Sorry, Ctrl+Alt+F wasn't fixed - it still doesn't work if the filter field in debugger is focused
Looks like that is an issue with the localization then.
Anyway, I'm changing this in bug 1204836.
Flags: needinfo?(steffen.wilberg)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: