416303 - Show default list, clear search, focus list when hitting escape from the search box

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Ed Lee :Mardak]

Madhava agrees that it would be good to clear the search and move focus to the list when the user hits escape.

Whether or not we close the window on escape is a separate issue.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Comment 2

[Madhava Enros [:madhava]]

I agree that I agree.  I, personally, don't think that the window should close on escape, given that that's not standard behaviour on any of our platforms, but that's what bug 306197 is for.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 302093 [details] [diff] [review]
v1

r=sdwilsh

This does, however, need a browser test before it can land ;)

Comment 4

[Ed Lee :Mardak]

Attachment: v1.1

Now with more mochitest! That wasn't so bad was it? ;) ... yes it was! Silly EventUtils and keydown/keypress/keyup...

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 302221 [details] [diff] [review]
v1.1

>+function test()
>+{
>+  // Close the UI if necessary
>+  let wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);
nit: line wrapping

>+  // Show the Download Manager UI
>+  Cc["@mozilla.org/download-manager-ui;1"].getService(Ci.nsIDownloadManagerUI).show();
ditto

>+
>+  // Wait for the UI to show up then do stuff
>+  waitForExplicitFinish();
>+  setTimeout(function() {
actually, setTimeout is bad, and sayrer will be angry with you.  You need to actually register a listener with the window watcher, and then register a DOMContentLoaded event like so:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/test/browser/browser_bug414214.js&rev=1.2#62

>+    win = wm.getMostRecentWindow("Download:Manager");
>+
>+    // Put in a value to clear out
>+    let search = win.document.getElementById("searchbox");
>+    search.value = "booga";
>+    search.focus();
>+
>+    let doEscape = function(aType) EventUtils.synthesizeKey("VK_ESCAPE", { type: aType }, win);
I'm not sure why you have to do all this.  You should talk to Enn about it.

Comment 6

[Ed Lee :Mardak]

Attachment: v1.2

80 characters max per line.. :p

Comment 7

[Ed Lee :Mardak]

Many hours later and I still can't get it to work with the window watcher. Some one else can take the bug.

Comment 8

[Shawn Wilsher :sdwilsh]

Enn - any idea why sythesizeKey doesn't work, but actually pressing the key does?

Comment 9

[Ed Lee :Mardak]

Attachment: v1.3

Meh. Meh. Meh. Clearly not in the greatest of moods. Meh.

Comment 10

[Shawn Wilsher :sdwilsh]

er, so, setTimeout with a timeout of 0 should be OK.  The ones with non-zero values are the problem.

Comment 11

[Ed Lee :Mardak]

Attachment: v1.4

Went with keypress instead of keydown to be consistent with what the keyset expects.

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 302340 [details] [diff] [review]
v1.4

>+/* ***** BEGIN LICENSE BLOCK *****
>+ *   Version: MPL 1.1/GPL 2.0/LGPL 2.1
is there normally that space there?

>+  // Show the Download Manager UI
>+  Cc["@mozilla.org/download-manager-ui;1"].getService(Ci.nsIDownloadManagerUI).
>+  show();
nit: this line wrapping looks weird

r=sdwilsh

Comment 13

[Ed Lee :Mardak]

Attachment: v1.5

(In reply to comment #12)
> >+/* ***** BEGIN LICENSE BLOCK *****
> is there normally that space there?
Yup.

> >+  // Show the Download Manager UI
> >+  Cc["@mozilla.org/download-manager-ui;1"].getService(Ci.nsIDownloadManagerUI).
> >+  show();
> nit: this line wrapping looks weird
Moved getService to the next line and combined show.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> > >+/* ***** BEGIN LICENSE BLOCK *****
> > is there normally that space there?
> Yup.
I meant the next line, because I've never seen the additional white space before it.

Comment 15

[Ed Lee :Mardak]

Oh. All the components/downloads/test/unit files have "* Version:..." while the mozapps/downloads/test/browser files have "*   Version:..."

Comment 16

[Shawn Wilsher :sdwilsh]

So that was probably my mistake at some point.  Could you fix that please with this patch?

Comment 17

[Ed Lee :Mardak]

Attachment: v1.6

Fixed up license boilerplate on other files. Seems like we copy/paste a lot ! ;) :p

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304799 [details] [diff] [review]
v1.6

a=beltzner for 1.9

Comment 19

[Ed Lee :Mardak]

Checking in toolkit/components/downloads/src/nsDownloadManagerUI.js;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManagerUI.js,v  <--  nsDownloadManagerUI.js
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/components/downloads/test/browser/browser_bug414214.js;
/cvsroot/mozilla/toolkit/components/downloads/test/browser/browser_bug414214.js,v  <--  browser_bug414214.js
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/downloads/test/browser/browser_nsIDownloadManagerUI.js;
/cvsroot/mozilla/toolkit/components/downloads/test/browser/browser_nsIDownloadManagerUI.js,v  <--  browser_nsIDownloadManagerUI.js
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.138; previous revision: 1.137
done
Checking in toolkit/mozapps/downloads/src/DownloadUtils.jsm;
/cvsroot/mozilla/toolkit/mozapps/downloads/src/DownloadUtils.jsm,v  <--  DownloadUtils.jsm
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/mozapps/downloads/tests/browser/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/Makefile.in,v  <--  Makefile.in
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_basic_functionality.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_basic_functionality.js,v  <--  browser_basic_functionality.js
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_394039.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_394039.js,v  <--  browser_bug_394039.js
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_410289.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_410289.js,v  <--  browser_bug_410289.js
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_411172.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_411172.js,v  <--  browser_bug_411172.js
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_411172_mac.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_411172_mac.js,v  <--  browser_bug_411172_mac.js
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_413985.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_413985.js,v  <--  browser_bug_413985.js
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_416303.js,v
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_416303.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_416303.js,v  <--  browser_bug_416303.js
initial revision: 1.1
done
Checking in toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js,v  <--  test_DownloadUtils.js
new revision: 1.2; previous revision: 1.1
done

Comment 20

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008022204 Minefield/3.0b4pre. Also verified on the Win XP nightly.