Closed Bug 827293 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- verified
firefox21 --- verified

People

(Reporter: mak, Assigned: mano)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Component: Downloads Panel → Bookmarks & History
Priority: -- → P2
Summary: Error: TypeError: view.selectNode is not a function → The first list item in the downloads view should be selected when it's opened (was: Error: TypeError: view.selectNode is not a function)
Attached patch patch (obsolete) — Splinter Review
Attachment #701223 - Flags: review?(mak77)
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);
Attachment #701223 - Flags: review?(mak77)
Attached patch patchSplinter Review
Attachment #701223 - Attachment is obsolete: true
Attachment #701241 - Flags: review?(mak77)
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
Attachment #701241 - Flags: review?(mak77) → review+
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
Attachment #701241 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f37caebbb0dc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Attachment #701241 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 830242
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.
Depends on: 868857
You need to log in before you can comment on or make changes to this bug.