570760 - Make ctrl-f and / focus the search box in the add-ons manager

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Whatever keyboard shortcuts normally open the find bar should focus the search box in the add-ons manager

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, is this bug blocked by bug 569342?

Comment 2

[Dave Townsend [:mossop]]

(In reply to comment #1)
> Dave, is this bug blocked by bug 569342?

I don't believe so

Comment 3

[Jennifer Morrow [:Boriss] (UX)]

I recommend not changing find-in-page search.  This is a known convention on every page in Firefox, used so often by most users that it's muscle memory.  Changing it on only one page would need to be done for quite a good reason, as it would surprise any user who already uses control-f and understands it to be a browser-wide convention. 

So, why would we change it here?  The argument is that if the user wants to do a search for a string, they might actually want to find a particular add-on and thus would be better off searching through all categories rather than the one they are looking at.  This defies user expectation by directing their focus to an area they wouldn't expect (add-on search rather than find-in-page bar) and then performing an action they aren't expecting (showing a list of results rather than highlighting matches on the current page).

However, what if the user actually wanted to search the text of the page?  That is, after all, what control-f does everywhere else, and the page is filled with searchable text.  Perhaps the user only wanted to look within their appearance of extensions view.  Maybe they've already narrowed down their search by going to this page.  For anyone familiar with control-f already, it seems likely they've pressed control-f with the intention of actually searching the page they are on.  What we'd be doing by changing this behavior is making an assumption about the user's intention while not producing the results they'd likely expect.

There's also a wider consistency issue here with the rest of Firefox.  Many sites, and eventually our own in-content pages, allow internal search.  By highlighting the internal search of the add-ons manager, we're implicitly also stating that "find in page" is more of a "find content" use than always a textual search.  When we put preferences in-content, we'd need to have no find-in-page search available there either.  Then you could make the argument that control-f merely takes you to the best way to find content on a page, rather than always looking for text.  To summon the spirit of Faaborg, this makes the behavior that results from control-f entirely modal and dependent on content. 

I'm marking this as wontfix - feel free to reopen if you'd like to discuss the above.

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> So, why would we change it here?

Because it is broken in the add-ons manager and so we are disabling the find bar in bug 569342 so it seemed reasonable that something sensible should happen when the user tried to use it. Since we can't search the page content, searching the add-ons contained in the manager seemed like a good idea.

Comment 5

[James Hobin [:hobinjk]]

Attachment: Patch making Cmd or Ctrl F and / focus the search bar in the Extension Manager

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 560836 [details] [diff] [review]
Patch making Cmd or Ctrl F and / focus the search bar in the Extension Manager

>diff --git a/browser/components/preferences/applications.xul b/browser/components/preferences/applications.xul
>--- a/browser/components/preferences/applications.xul
>+++ b/browser/components/preferences/applications.xul
>@@ -104,17 +104,17 @@
>     </preferences>
> 
>     <script type="application/javascript" src="chrome://browser/content/preferences/applications.js"/>
> 
>     <keyset>
>       <key key="&focusSearch1.key;" modifiers="accel" oncommand="gApplicationsPane.focusFilterBox();"/>
>       <key key="&focusSearch2.key;" modifiers="accel" oncommand="gApplicationsPane.focusFilterBox();"/>
>     </keyset>
>-
>+	
>     <hbox>
>       <textbox id="filter" flex="1"
>                type="search"
>                placeholder="&filter.emptytext;"
>                aria-controls="handlersView"
>                oncommand="gApplicationsPane.filter();"/>
>     </hbox>

unrelated change crept in

>diff --git a/browser/config/mozconfig b/browser/config/mozconfig
>--- a/browser/config/mozconfig
>+++ b/browser/config/mozconfig
>@@ -1,5 +1,12 @@
> # This file specifies the build flags for Firefox.  You can use it by adding:
>-#  . $topsrcdir/browser/config/mozconfig
>+. $topsrcdir/browser/config/mozconfig
> # to the top of your mozconfig file.
>+mk_add_options MOZ_OBJDIR=@TOPSCRDIR@/ff-opt-libxul
>+ac_add_options --enable-optimize
>+ac_add_options --enable-libxul
>+ac_add_options --enable-shark
>+ac_add_options --enable-debug-symbols
>+ac_add_options --enable-debugger-info-modules
>+ac_add_options --disable-install-strip
> 
>-ac_add_options --enable-application=browser
>+# ac_add_options --enable-application=browser

ditto

>--- a/toolkit/mozapps/extensions/content/extensions.js
>+++ b/toolkit/mozapps/extensions/content/extensions.js
>@@ -1714,17 +1714,21 @@ var gHeader = {
> 
>     window.addEventListener("focus", function(aEvent) {
>       if (aEvent.target == window)
>         updateNavButtonVisibility();
>     }, false);
> 
>     updateNavButtonVisibility();
>   },
>-
>+  focusSearchBox: function() {
>+  	this._search.focus();
>+  },
>+  	
>+  	

nit: you're adding trailing spaces here

>--- a/toolkit/mozapps/extensions/content/extensions.xul
>+++ b/toolkit/mozapps/extensions/content/extensions.xul
>@@ -51,25 +51,25 @@
> <page xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>       xmlns:xhtml="http://www.w3.org/1999/xhtml"
>       id="addons-page" title="&addons.windowTitle;"
>       role="application" windowtype="Addons:Manager"
>       disablefastfind="true"
>       ondragenter="gDragDrop.onDragOver(event)"
>       ondragover="gDragDrop.onDragOver(event)"
>       ondrop="gDragDrop.onDrop(event)">
>-
>+	
>   <xhtml:link rel="shortcut icon"
>               href="chrome://mozapps/skin/extensions/extensionGeneric-16.png"/>
> 
>   <script type="application/javascript"
>           src="chrome://mozapps/content/extensions/extensions.js"/>
>   <script type="application/javascript"
>           src="chrome://global/content/contentAreaUtils.js"/>
>-
>+  

ditto

>+    	<key id="focusSearch1" key="F" modifiers="accel" oncommand="gHeader.focusSearchBox();"/>

This should be localized, à la <http://mxr.mozilla.org/mozilla-central/search?string=findOnCmd.commandkey>.

Comment 7

[James Hobin [:hobinjk]]

Attachment: Update to patch, adding localization and removing crept in edits

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 560896 [details] [diff] [review]
Update to patch, adding localization and removing crept in edits

>+<!ENTITY focusSearch.key1					  "F">

search.commandkey?

>+<!ENTITY focusSearch.key2					  "/">

This is hardcoded in findbar.xml, so I guess it shouldn't be localized.

>--- a/toolkit/mozapps/extensions/content/extensions.js
>+++ b/toolkit/mozapps/extensions/content/extensions.js

>+  focusSearchBox: function() {
>+  	this._search.focus();
>+  },
>+  

this._search.focus(); is wrongly indented with a TAB (use two spaces instead).
There are still two trailing spaces on the empty line.

>+    	
>+    </keyset>
>     <textbox id="header-search" type="search" searchbutton="true"
>              placeholder="&search.placeholder;"/>
>+             
>   </hbox>
>-
>   <hbox flex="1">

More whitespace fail.

Comment 9

[James Hobin [:hobinjk]]

Attachment: Fixing formatting and naming considerations

Hopefully everything here is correct this time

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 560919 [details]
Fixing formatting and naming considerations

>+<!-- search bar focus function key -->
>+<!ENTITY search.commandkey					  "F">

Still too much indentation / tabs.
I'd probably move this directly after <!ENTITY search.placeholder...

>   },
>-
>+  focusSearchBox: function() {
>+    this._search.focus();
>+  },
>   get shouldShowNavButtons() {

nit: add a blank line (no spaces) before and after the mothod

>+    <keyset id="headerSearchKeyset">

Can you move this up to the <commandset>s?
I don't think this needs an id...

>+        <key id="focusSearch1" key="&search.commandkey;" modifiers="accel" oncommand="gHeader.focusSearchBox();"/>

This in indented two spaces two far. The line is also overly long, should be wrapped like this:

      <key id="focusSearch1" key="&search.commandkey;" modifiers="accel"
           oncommand="gHeader.focusSearchBox();"/>

>+        <key id="focusSearch2" key="/" oncommand="gHeader.focusSearchBox();"/>

This doesn't work over here, as "/" is Shift+7 on my keyboard. I tried keycode="VK_SLASH" instead of key="/", but this doesn't seem to work. Adding modifiers="shift" to key="/" fixes it for me but would presumably break it for other keyboard layouts. The only way around this is probably to avoid <key> and use a keypress event handler instead, like findbar.xml is doing: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#1412

Comment 11

[James Hobin [:hobinjk]]

Attachment: Bug 570760 - Fixing previous patch to use onkeypressed javascript for slash key and be properly formatted

I updated it to use pretty redundant javascript, hopefully this will solve the shift-7 issue ( and hopefully there are no trailing spaces )

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 561088 [details] [diff] [review]
Bug 570760 - Fixing previous patch to use onkeypressed javascript for slash key and be properly formatted

>+<!ENTITY search.commandkey                    "F">

nit: lowercase f

>+  onKeyPress: function( aEvent ) {
>+    if( aEvent.keyCode ) {
>+      if( aEvent.keyCode == KeyEvent.DOM_VK_SLASH ) {
>+        this.focusSearchBox();
>+      }
>+    } else if( String.fromCharCode(aEvent.charCode) == "/" ) {
>+      this.focusSearchBox();
>+    }
>+  },

Are both branches really needed?

Note on the style: "if (xxx)" instead of "if( xxx )"

> <page xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>       xmlns:xhtml="http://www.w3.org/1999/xhtml"
>       id="addons-page" title="&addons.windowTitle;"
>       role="application" windowtype="Addons:Manager"
>       disablefastfind="true"
>       ondragenter="gDragDrop.onDragOver(event)"
>       ondragover="gDragDrop.onDragOver(event)"
>-      ondrop="gDragDrop.onDrop(event)">
>+      ondrop="gDragDrop.onDrop(event)"
>+      onkeypress="gHeader.onKeyPress(event)">

gHeader strikes me as an unusual place for a generic keypress handler, but as long as it only cares about search, I guess it's not a big deal and it can be moved elsewhere if and when it handles something besides search.

>+  <keyset id="focusSearch">
>+    <key key="&search.commandkey;" modifiers="accel"
>+         oncommand="gHeader.focusSearchBox();"/>
>+  </keyset>

Did you mean to set id="focusSearch" on the <key> rather than the <keyset>?

Comment 13

[James Hobin [:hobinjk]]

Attachment: Bug 570760 - Fixing formatting and capitalization

I think this update might end the saga of localization and trailing spaces that my first real ( more than one line ) patch has been.

As far as the redundancy of the Javascript, I put that in there because I was unsure if different keyboard layouts would cause the slash to be passed as a char or key code.

Comment 14

[Dão Gottwald [:dao]]

(In reply to James Hobin from comment #13)
> As far as the redundancy of the Javascript, I put that in there because I
> was unsure if different keyboard layouts would cause the slash to be passed
> as a char or key code.

Both branches (keyCode and charCode) appear to work over here.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 561380 [details] [diff] [review]
Bug 570760 - Fixing formatting and capitalization

>+  onKeyPress: function( aEvent ) {

nit: remove spaces around aEvent

I would put a space between 'function' and '(', but the prevailing style in this file disagrees with me.

Comment 16

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to James Hobin from comment #13)
> > As far as the redundancy of the Javascript, I put that in there because I
> > was unsure if different keyboard layouts would cause the slash to be passed
> > as a char or key code.
> 
> Both branches (keyCode and charCode) appear to work over here.

I take that back, only charCode works. findbar.xml uses charCode, too.

Comment 17

[James Hobin [:hobinjk]]

Okay, I was wondering how both charCode and keyCode could work if the documentation for the DOM event creation said that only one would be set at a time, although that wouldn't have been the first time something non-standard showed up in a browser.

Comment 18

[Dão Gottwald [:dao]]

So what do we need keyCode for? Does charCode not work for you?

Comment 19

[James Hobin [:hobinjk]]

I thought that keyCode might come in to play with different locales, but since it works with charCode for you as well I'll remove the unnecessary  branch.

Comment 20

[James Hobin [:hobinjk]]

Attachment: Bug 570760 - Removing unnecessary logic

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 562342 [details] [diff] [review]
Bug 570760 - Removing unnecessary logic

>+  onKeyPress: function( aEvent ) {

see comment 15

Comment 22

[James Hobin [:hobinjk]]

Attachment: Bug 570760 - Adding focusing functionality to addons manager - Fixing spaces

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 564015 [details] [diff] [review]
Bug 570760 - Adding focusing functionality to addons manager - Fixing spaces

Thanks! :)

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Patch looks good - thanks :)

James, would you be willing to write a test for this?

It would go in toolkit/mozapps/extensions/test/browser/ - lots of examples to go by in that directory. General documentation on browser-chrome tests at https://developer.mozilla.org/en/Browser_chrome_tests

Comment 25

[James Hobin [:hobinjk]]

I'll certainly write a test!
I'm looking at the directory of tests now. I will admit that the wiki's test writing documentation is minimal. Perhaps a tutorial on writing them ( maybe even based on this ) is in order...

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to James Hobin from comment #25)
> I will admit that the wiki's test writing documentation is minimal.

Yea :( Let us know if you need help or get stuck.

> Perhaps a tutorial on writing them ( maybe even based on this ) is in order...

That sounds like a great idea :)

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 564015 [details] [diff] [review]
Bug 570760 - Adding focusing functionality to addons manager - Fixing spaces

(Putting r- on this to make it clear it's not done yet, as per comment 25.)

Comment 28

[James Hobin [:hobinjk]]

For some reason my hg queue is not seeing the new browser_bug570760.js, but is registering the fact that I added it to the Makefile.in, any idea why this might be happening?

Comment 29

[James Hobin [:hobinjk]]

Attachment: Adding mochitest functionality

I did some terrible things to my mercurial repository by not realizing some very important things ( qnew vs. qref ), but hopefully this patch is formatted properly ( generated using export after several qfolds )

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 565851 [details] [diff] [review]
Adding mochitest functionality

>+    searchBox.addEventListener("focus", function() {

You should probably remove this listener when you don't need it anymore in order to not keep the document alive.

>+      focusCount ++;

focusCount++;

>+  close_manager(gManagerWindow, function() {
>+    finish();
>+  });

close_manager(gManagerWindow, finish);

Comment 31

[James Hobin [:hobinjk]]

Attachment: Adding mochitest functionality, fixing things

I've been having some issues with mercurial being special, so I just added the changes to the patch file. Hopefully that worked, but otherwise I will be working on getting the version control to not be a series of explosions.

Comment 32

[James Hobin [:hobinjk]]

Attachment: Bug 570760 - Adding mochitest! ( proper hg syntax )

This revision of the patch I did properly using hg export, not whatever weird thing I did before, so it is more likely to work.

Comment 33

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 568664 [details] [diff] [review]
Bug 570760 - Adding mochitest! ( proper hg syntax )

Sorry for the delay... hoping Dave can review this faster than I can.

Comment 34

[Dave Townsend [:mossop]]

Comment on attachment 568664 [details] [diff] [review]
Bug 570760 - Adding mochitest! ( proper hg syntax )

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

One minor change but this looks fine otherwise, thanks!

::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
@@ +1,3 @@
>  <!ENTITY addons.windowTitle                   "Add-ons Manager">
>  <!ENTITY search.placeholder                   "Search all add-ons">
> +<!ENTITY search.commandkey                    "f">

Add a localization note that this should match findOnCmd.commandkey from browser.dtd

Comment 35

[James Hobin [:hobinjk]]

Attachment: Patch adding localization note.

I added a localization note based on the other one, but I'm not sure if the placement is quite proper. Also, the way Mercurial handled the patch export is rather odd.

Comment 36

[Dave Townsend [:mossop]]

(In reply to James Hobin from comment #35)
> Created attachment 571242 [details] [diff] [review] [diff] [details] [review]
> Patch adding localization note.
> 
> I added a localization note based on the other one, but I'm not sure if the
> placement is quite proper. Also, the way Mercurial handled the patch export
> is rather odd.

Looks like you've just exported two separate patches there or something, it'd be useful to have it all in one. Also please note the format of localization notes like this one http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd#41 put the string that the note is for in brackets and the comment should be placed immediately before the appropriate string where possible.

Comment 37

[Dave Townsend [:mossop]]

Comment on attachment 571242 [details] [diff] [review]
Patch adding localization note.

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

r- per previous comment

Comment 38

[James Hobin [:hobinjk]]

Attachment: Bug 570760 - Reformatting localization note, fixing patch format

Making the patch diff "pretty" turned out to be far more interesting than intended. Because I had previously followed bad practices in making my patches, I ended up having to re-clone the entire repository.

Comment 39

[Dave Townsend [:mossop]]

Comment on attachment 576083 [details] [diff] [review]
Bug 570760 - Reformatting localization note, fixing patch format

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

One minor issue to correct then this can land.

::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
@@ +3,3 @@
>  <!ENTITY search.placeholder                   "Search all add-ons">
> +<!-- LOCALIZATION NOTE (search.commandKey):
> ++     The search command key should match findOnCmd.commandkey from browser.dtd -->

This seems to have a + at the start of the line that shouldn't be there.

Comment 40

[James Hobin [:hobinjk]]

Attachment: Bug 570760 - Plus sign removed

The plus sign is what I get for trying to play the role of hg while rebuilding the repository....oops.

Comment 41

[Dave Townsend [:mossop]]

Comment on attachment 578424 [details] [diff] [review]
Bug 570760 - Plus sign removed

No need for another review here, it already has r+

Comment 42

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f8a25d495522

Comment 43

[Dão Gottwald [:dao]]

The test is broken. I'm trying to fix it here: http://hg.mozilla.org/integration/mozilla-inbound/rev/2b7bcf902f8d
If it still fails after that, the patch will need to be backed out.

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a02f77397320
https://hg.mozilla.org/mozilla-central/rev/2b7bcf902f8d