827293 - The first list item in the downloads view should be selected when it's opened (was: Error: TypeError: view.selectNode is not a function)

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Marco Bonardo [:mak]]

this happens when opening the downloads view the first time

Error: TypeError: view.selectNode is not a function
Source file: chrome://browser/content/places/places.js
Line: 64

Mano points out registration of the session downloads is async and this may be a problem for a11y, blocking based on that, will re-evaluate later.

Comment 1

[Asaf Romano (gone)]

Attachment: patch

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 701223 [details] [diff] [review]
patch

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

not + for the typo mostly :)

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +1103,5 @@
>      }
>  
>      this._appendDownloadsFragment(elementsToAppendFragment);
>      this._ensureVisibleElementsAreActive();
> +    this._ensureInitialSelction();

typo, copypasted around "Selction"

I honestly don't like much that this happens on invalidateContainer, since that may happen at any time, not just on startup... couldn't we do this on load? or that wouldn't work cause PB doesn't have a place result? If so would be good to document that.

@@ +1181,5 @@
> +    // the order in which the data comes in.
> +    // We work around this by attempting to select the first element twice,
> +    // once after the places data is loaded and once when the session downloads
> +    // data is done loading.  However, if the selection has changed in-between,
> +    // we assume the user has already started using the view and give up.

this documentation should be moved to a proper javadoc for this helper.

@@ +1186,4 @@
>  
> +    // Either they're both null, or the selection has not changed in between.
> +    if (this._richlistbox.selectedItem == this._initiallySelectedElement) {
> +      let firstDownloadElement = this._richlistbox.querySelector("richlistitem.download");

as discussed on IRC, for now use firstChild. we'll use selectors when there will be the need to use selectors.

@@ +1192,5 @@
> +        // or before the download binding is attached. Therefore, ensure the
> +        // first item is activated, and that we only pass it to the richlistbox
> +        // binding when it has the richlistitem binding attached.
> +        firstDownloadElement._shell.ensureActive();
> +        setTimeout(function() {

please document this enqueue better if possible, the above comment doesn't seem to point out the need of enqueuing as it is... and instead of setTimeout(0) I'd prefer Services.tm.mainThread.dispatch(_function_, Ci.nsIThread.DISPATCH_NORMAL);

Comment 3

[Asaf Romano (gone)]

Attachment: patch

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 701241 [details] [diff] [review]
patch

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

I had worse nightmares...

Please before pushing run browser-chrome tests in browser/components/downloads/tests/ since this changes the behavior of the view and I want to be sure won't fail there

Comment 5

[Asaf Romano (gone)]

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

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 701241 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Downloads panel feature
User impact if declined: incomplete UI
Testing completed (on m-c, etc.): m-i, local testing
Risk to taking this patch (and alternatives if risky): limited to the feature
String or UUID changes made by this patch: none

Comment 7

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/f37caebbb0dc

Comment 8

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f9deba82d43b

Comment 9

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Windows NT 6.2; rv:20.0) Gecko/20130204 Firefox/20.0
Build ID: 20130204042019
Mozilla/5.0 (Windows NT 6.2; rv:21.0) Gecko/20130204 Firefox/21.0
Build ID: 20130204030941
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8: rv:20.0) Gecko/20130204 Firefox/20.0
Build ID: 20130204042019
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8: rv:21.0) Gecko/20130204 Firefox/21.0
Build ID: 20130204030941

Verified as fixed using Windows 8 and Mac OS X 10.8, on latest Nightly and Aurora.