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

RESOLVED FIXED in mozilla11

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: mossop, Assigned: hobinjk)

Tracking

(Blocks: 1 bug)

Trunk
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [AddonsRewrite][good first bug])

Attachments

(1 attachment, 12 obsolete attachments)

6.17 KB, patch
mossop
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
Whatever keyboard shortcuts normally open the find bar should focus the search box in the add-ons manager
blocking2.0: --- → ?

Updated

8 years ago
Blocks: 563909
(Reporter)

Updated

8 years ago
blocking2.0: ? → -
Dave, is this bug blocked by bug 569342?
Whiteboard: [AddonsRewrite] → [AddonsRewrite][good first bug]
(Reporter)

Comment 2

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

I don't believe so
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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 4

7 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Status: REOPENED → NEW

Updated

6 years ago
Blocks: 682514

Updated

6 years ago
No longer blocks: 682514
(Assignee)

Comment 5

6 years ago
Created attachment 560836 [details] [diff] [review]
Patch making Cmd or Ctrl F and / focus the search bar in the Extension Manager
Attachment #560836 - Flags: review?(dtownsend)
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>.
Attachment #560836 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 7

6 years ago
Created attachment 560896 [details] [diff] [review]
Update to patch, adding localization and removing crept in edits
Attachment #560836 - Attachment is obsolete: true
Attachment #560896 - Flags: review?(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.
Attachment #560896 - Flags: review?(dao) → review-

Updated

6 years ago
Assignee: nobody → hobinjk
Status: NEW → ASSIGNED
(Assignee)

Comment 9

6 years ago
Created attachment 560919 [details]
Fixing formatting and naming considerations

Hopefully everything here is correct this time
Attachment #560896 - Attachment is obsolete: true
Attachment #560919 - Flags: review?(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
Attachment #560919 - Flags: review?(dao) → review-
(Assignee)

Comment 11

6 years ago
Created attachment 561088 [details] [diff] [review]
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 )
Attachment #560919 - Attachment is obsolete: true
Attachment #561088 - Flags: review?(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>?
(Assignee)

Comment 13

6 years ago
Created attachment 561380 [details] [diff] [review]
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.
Attachment #561088 - Attachment is obsolete: true
Attachment #561380 - Flags: review?(dao)
Attachment #561088 - Flags: review?(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 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.
(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.
(Assignee)

Comment 17

6 years ago
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.
So what do we need keyCode for? Does charCode not work for you?
(Assignee)

Comment 19

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

Comment 20

6 years ago
Created attachment 562342 [details] [diff] [review]
Bug 570760 - Removing unnecessary logic
Attachment #561380 - Attachment is obsolete: true
Attachment #562342 - Flags: review?(dao)
Attachment #561380 - Flags: review?(dao)
Comment on attachment 562342 [details] [diff] [review]
Bug 570760 - Removing unnecessary logic

>+  onKeyPress: function( aEvent ) {

see comment 15
(Assignee)

Comment 22

6 years ago
Created attachment 564015 [details] [diff] [review]
Bug 570760 - Adding focusing functionality to addons manager - Fixing spaces
Attachment #562342 - Attachment is obsolete: true
Attachment #564015 - Flags: review?(dao)
Attachment #562342 - Flags: review?(dao)
Comment on attachment 564015 [details] [diff] [review]
Bug 570760 - Adding focusing functionality to addons manager - Fixing spaces

Thanks! :)
Attachment #564015 - Flags: review?(dao)
Attachment #564015 - Flags: review?(bmcbride)
Attachment #564015 - Flags: review+
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
(Assignee)

Comment 25

6 years ago
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...
(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 :)
Attachment #564015 - Flags: review?(bmcbride)
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.)
Attachment #564015 - Flags: review-
(Assignee)

Comment 28

6 years ago
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?
(Assignee)

Comment 29

6 years ago
Created attachment 565851 [details] [diff] [review]
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 )
Attachment #564015 - Attachment is obsolete: true
Attachment #565851 - Flags: review?(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);
(Assignee)

Comment 31

6 years ago
Created attachment 566075 [details] [diff] [review]
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.
Attachment #565851 - Attachment is obsolete: true
Attachment #566075 - Flags: review?(dao)
Attachment #565851 - Flags: review?(dao)
(Assignee)

Comment 32

6 years ago
Created attachment 568664 [details] [diff] [review]
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.
Attachment #566075 - Attachment is obsolete: true
Attachment #568664 - Flags: review?(dao)
Attachment #566075 - Flags: review?(dao)

Updated

6 years ago
Attachment #568664 - Flags: review?(dao) → review?(bmcbride)
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.
Attachment #568664 - Flags: review?(bmcbride) → review?(dtownsend)
(Reporter)

Comment 34

6 years ago
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
Attachment #568664 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 35

6 years ago
Created attachment 571242 [details] [diff] [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.
Attachment #568664 - Attachment is obsolete: true
Attachment #571242 - Flags: review?(dtownsend)
(Reporter)

Comment 36

6 years ago
(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.
(Reporter)

Comment 37

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

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

r- per previous comment
Attachment #571242 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 38

6 years ago
Created attachment 576083 [details] [diff] [review]
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.
Attachment #571242 - Attachment is obsolete: true
Attachment #576083 - Flags: review?(dtownsend)
(Reporter)

Comment 39

6 years ago
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.
Attachment #576083 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 40

6 years ago
Created attachment 578424 [details] [diff] [review]
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.
Attachment #576083 - Attachment is obsolete: true
Attachment #578424 - Flags: review?(dtownsend)
(Reporter)

Comment 41

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

No need for another review here, it already has r+
Attachment #578424 - Flags: review?(dtownsend) → review+

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/f8a25d495522
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla11
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.
https://hg.mozilla.org/mozilla-central/rev/a02f77397320
https://hg.mozilla.org/mozilla-central/rev/2b7bcf902f8d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → FIXED

Updated

2 years ago
Blocks: 1195060
You need to log in before you can comment on or make changes to this bug.