Closed Bug 285584 Opened 20 years ago Closed 19 years ago

Make download & extension manager use accessible XBL widget

Categories

(Toolkit :: Downloads API, defect)

Product:
Toolkit
Component:
Downloads API
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: doronr)

References

Details

(Keywords: access)

Attachments

(1 file, 7 obsolete files)

with mconnor's comments
(deleted), patch
mconnor
: review+
benjamin
: approval-aviary1.1a2+
Details | Diff | Splinter Review
The accessibility module has no what these vbox and hbox based widgets are or when an item gets focused, etc. One option is to use an existing XBL-based XUL widget like listbox. At the very least these download manager extension manager needs to be made into a new widget in XBL so that I can use nsIAccessibilityService to create a new kind of accessible for it.
Flags: blocking-aviary1.1?
As far as I understand using <listbox> will also fix bug 235203 and bugs depending on it.
Hardware: PC → All
Version: unspecified → Trunk
I guess the theme manager might be another candidate.
Blocks: 281538
Blocks: 235203
Blocks: 247065
taking. I am currently calling it richlistbox, as it is intended to be able to hold any XUL content in each item. Using a xul:scrollbox currently, which seems to give us scrollIntoView type functionality for free :)
Assignee: bugs → doronr
Doron, are you sure that the current <listbox> implementation can't be extended to do what we need?. Writing something completely from scratch might be more work than just making a <listbox type="rich"> which extends listbox. What about all the features that will need to be implemented, such as multiple selection, context menu support and incremental search as you type? Can we get any of those for free? Acutally I thought the old DOM node based tree implementation could have arbitrary content, and it is still available, and also that <listbox> still basically uses that implementation. Not true? The accessible toolkit checklist might be a good place to look for somet hings that a list needs to be able to support, see item #5: http://www.mozilla.org/access/toolkit-checklist
(In reply to comment #4) > Doron, are you sure that the current <listbox> implementation can't be extended > to do what we need?. Writing something completely from scratch might be more > work than just making a <listbox type="rich"> which extends listbox. What about > all the features that will need to be implemented, such as multiple selection, > context menu support and incremental search as you type? Can we get any of those > for free? None of the ff windows need multi select or search as you type (which would be hard, as the content could be anything, rather than searching value/label attributes). I'll have to check context menus, but those should not matter, since the content inside each rich row uses xul elements that support them. > > Acutally I thought the old DOM node based tree implementation could have > arbitrary content, and it is still available, and also that <listbox> still > basically uses that implementation. Not true? trees are pretty much limited to one row. This new widget basically does what all the ff windows that use the funky listbox like thing should have done in the first place ;)
> None of the ff windows need multi select or search as you type Excuse me for horning in, but multiple selection would be useful for Extension Manager (bug 259057). Find-as-you-type would be nice to have too (iirc, this is how old Tools > Options > Extensions list worked).
(In reply to comment #6) > > None of the ff windows need multi select or search as you type > Excuse me for horning in, but multiple selection would be useful for Extension > Manager (bug 259057). Find-as-you-type would be nice to have too (iirc, this is > how old Tools > Options > Extensions list worked). That is adding new features, which is not in the scope of this bug. But adding multi-select would be trivial, and I'll try to make it easy to add later.
Here is what I have right now: http://www.nexgenmedia.net/a11y/fancy-widget/fancy.xul Seems to work ok (much better than what firefox has now).
Attached patch what I currently have (obsolete) (deleted) — — Splinter Review
work in progress
Attached patch Works good, needs testing and code comments (obsolete) (deleted) — — Splinter Review
Attachment #180616 - Attachment is obsolete: true
cc: toolkit people for comments :)
Status: NEW → ASSIGNED
Binding from content/widgets are expected to work in any xul document... that's: use xul.css instead of download/extension.css.
Comment on attachment 180826 [details] [diff] [review] Works good, needs testing and code comments >+ <field name="selectedItem">null</field> >+ <method name="setSelectedItem"> >+ <parameter name="aElement"/> >+ <parameter name="aSetFocus"/> >+ <body> >+ <![CDATA[ >+ if (this.selectedItem) >+ this.selectedItem.selected = false; >+ >+ this.selectedItem = aElement; >+ >+ if (aElement) { >+ aElement.selected = true; >+ if (aSetFocus) >+ aElement.focus(); >+ this.scrollObject.ensureElementIsVisible(aElement); >+ } >+ >+ this.fireEvent("select"); >+ ]]> >+ </body> >+ </method> >+ >+ <method name="getSelectedItem"> >+ <body> >+ return this.selectedItem; >+ </body> >+ </method> XBL has a useful <property> element ;) (see listbox.xml)
> > > XBL has a useful <property> element ;) (see listbox.xml) > well, my setter needs 2 arguments, so I went with a method for the getter as well (in theory, methods will be needed for muli select if it ever happens) :)
Comment on attachment 180826 [details] [diff] [review] Works good, needs testing and code comments neil gave good comments on irc, should see a new version tuesday.
Attachment #180826 - Attachment is obsolete: true
Attached patch Fix comments from Neil (obsolete) (deleted) — — Splinter Review
Attachment #181211 - Flags: review?(neil.parkwaycc.co.uk)
Shouldn't the up/down keypress handlers call event.preventDefault()? I didn't test the latest patch, but the demo, as well as current managers also scroll a bit on each up/down keypress, and it doesn't seem this was changed in the patch.
Blocks: deera11y
(In reply to comment #17) > Shouldn't the up/down keypress handlers call event.preventDefault()? I didn't > test the latest patch, but the demo, as well as current managers also scroll a > bit on each up/down keypress, and it doesn't seem this was changed in the patch. What do you mean by scroll a bit on each up/down? Do you mean the fact that when you say go down, it scrolls, even if the newly selected item was fully visible? That is an issue in scrollBoxObject, and I rather make this accessible first and then try to fix scrollBoxObject in a seperate bug. Not sure if preventDefault would help, might try that.
Actually the up/down is caused by platformHTMLBindings.xml's bindings generating up/down scroll commands but calling event.preventDefault() will suppress them.
Attached patch prevent up/down events (obsolete) (deleted) — — Splinter Review
I learn something new every day!
Attachment #181211 - Attachment is obsolete: true
Attachment #181293 - Flags: review?(neil.robertson-ravo)
Attachment #181293 - Flags: review?(neil.robertson-ravo) → review?(neil.parkwaycc.co.uk)
Attachment #181211 - Flags: review?(neil.parkwaycc.co.uk)
mconnor: feel free to pitch in :)
Neil, it looks like Doron responded to your comments about up/down with a new patch. Still awaiting review -- does the new patch look good?
This patch has bitrotted, and the new extension manager changes are causing trouble. So this bug is in libo right now until EM stabilizes.
(In reply to comment #23) > This patch has bitrotted, and the new extension manager changes are causing > trouble. > > So this bug is in libo right now until EM stabilizes. Lost part of the message: The issue is that in the extension manager, if you add a new extension, it seems the whole DOM is rebuilt for the tree, and thus selection is lost. Rob Strong is working on fixing the EM, for example bug 292619. I can attach a new patch that has that issue, or we can wait for him to finish off his patches to fix this.
I'd really like to get this landed so if this can be landed soon after 1.1a1 I will be more than happy to update any of the other patches I have in the queue. At this point I think that bug 284515 is the main one that falls into this category and retaining the selected item can be sorted out quickly after this lands or in the same patch if you prefer. FWIW: In my tree I added disabled as an implementation property to richlistitem in richlistbox.xml to allow easy access to it from extensions.js.
Attached patch trunk patch (obsolete) (deleted) — — Splinter Review
Attachment #181293 - Attachment is obsolete: true
Attachment #185016 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #181293 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #185016 - Attachment is obsolete: true
Attachment #185016 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch readded some dnd stuff, robstrong caught that. (obsolete) (deleted) — — Splinter Review
neil - will you be able to review it, or should I find someone else?
Attachment #185040 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #185040 - Attachment is obsolete: true
Attachment #185040 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch applies to trunk (extensions.js changed) (obsolete) (deleted) — — Splinter Review
Since neil seems busy, asking our friendly neighbourhood reviewer, mconnor.
Attachment #185578 - Flags: review?(mconnor)
Comment on attachment 185578 [details] [diff] [review] applies to trunk (extensions.js changed) + // if nothing selected, we go from the bottom + for (var i = this.selectedItem ? this.selectedItem.previousSibling : this.lastChild; i; i = i.previousSibling) { + // if nothing selected, we go from the top + for (var i = (this.selectedItem != null) ? this.selectedItem.nextSibling : this.firstChild; i; i = i.nextSibling) { er, why didn't you use this.selectedItem by itself like you did previously? + <handlers> + <handler event="keypress" keycode="VK_UP" action="goUp(); event.preventDefault();"/> + <handler event="keypress" keycode="VK_DOWN" action="goDown(); event.preventDefault();"/> + <handler event="click"> + <![CDATA[ + // clicking into nothing should unselect + if (event.originalTarget.getAttribute("anonid") == "main-box") + this.clearSelection(); + ]]> + </handler> + <handler event="contextmenu"> + <![CDATA[ + + // if the context menu was opened via the keyboard, display it in the + // right location. extra line + if (event.button != 2) { + var popup = document.getElementById(this.getAttribute("context")); + if (popup) + popup.showPopup(this.selectedItem, -1, -1, "context", "bottomleft", "topleft"); + } isn't context menu modifier-click on Mac? if that's the case, this will act really weird on Mac. + <handlers> + <handler event="click"> + <![CDATA[ + if ((event.target == this) && event.ctrlKey && (this.parentNode.selectedItem == this)) { + this.parentNode.clearSelection(); + } else { + this.parentNode.selectedItem = this; + } + ]]> I wonder if this binding should support multi-select? It would have to be controllable via a property, but for dlmgr this would have a lot of value (selectively clear a range of items). Not expecting it as part of this bug, but please file a followup on expanding the capability of the binding. -#downloadView { - -moz-binding: url('chrome://mozapps/content/downloads/download.xml#download-view'); +/* Only focus links in the selected item*/ +download[selected="true"] .link { + -moz-user-focus: normal; } so the idea here is that you can only tab to links in the current listitem, and you have to use up/down to move to other downloads? I think I can live with that. Index: toolkit/mozapps/downloads/content/downloads.js =================================================================== RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v retrieving revision 1.43 diff -u -r1.43 downloads.js --- toolkit/mozapps/downloads/content/downloads.js 12 Apr 2005 15:55:49 -0000 1.43 +++ toolkit/mozapps/downloads/content/downloads.js 7 Jun 2005 16:59:50 -0000 @@ -220,6 +220,9 @@ // Add this download to the percentage average tally var dl = aSubject.QueryInterface(Components.interfaces.nsIDownload); gActiveDownloads.push(dl); + // clear the richlistitem + gDownloadsView.clearSelection(); + gDownloadsView.focus(); why? break; case "xpinstall-download-started": var windowArgs = aSubject.QueryInterface(Components.interfaces.nsISupportsArray); @@ -261,6 +264,10 @@ f.remove(false); gDownloadViewController.onCommandUpdate(); + + // clear the richlistitem + gDownloadsView.clearSelection(); + gDownloadsView.focus(); why? } function onDownloadPause(aEvent) @@ -269,11 +276,18 @@ gDownloadManager.pauseDownload(uri); setRDFProperty(uri, "DownloadStatus", aEvent.target.getAttribute("status-internal")); setRDFProperty(uri, "ProgressPercent", aEvent.target.getAttribute("progress")); + + // clear the richlistitem + gDownloadsView.clearSelection(); + gDownloadsView.focus(); } if we're pausing here, why kill the selection? function onDownloadResume(aEvent) { gDownloadManager.resumeDownload(aEvent.target.id); + // clear the richlistitem + gDownloadsView.clearSelection(); + gDownloadsView.focus(); } why? function onDownloadRemove(aEvent) @@ -282,6 +296,10 @@ gDownloadManager.removeDownload(aEvent.target.id); gDownloadViewController.onCommandUpdate(); + + // clear the richlistitem + gDownloadsView.clearSelection(); + gDownloadsView.focus(); } } is this really what we want to do in this case? I would expect that removing an item here should follow more typical listbox/file manager behaviour, not reset focus. I know that's not what the current dlmgr does, but we need to support this, here if not in the binding @@ -932,12 +932,10 @@ return; var selectedID = aSelectedItem.id; - var selectedElement = document.getElementById(selectedID); - var nextElement = selectedElement.nextSibling; - if (!nextElement) - nextElement = selectedElement.previousSibling; - nextElement = nextElement.id; - + // if no next item, go to the previous one + if (!gExtensionsView.goDown()) + gExtensionsView.goUp(); + ok, right, this is what we want to do in dlmgr if we don't want to make it explicit in the binding. easy as pie. Index: toolkit/themes/winstripe/mozapps/extensions/extensions.css =================================================================== RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/extensions.css,v retrieving revision 1.11 diff -u -r1.11 extensions.css --- toolkit/themes/winstripe/mozapps/extensions/extensions.css 22 May 2005 20:47:21 -0000 1.11 +++ toolkit/themes/winstripe/mozapps/extensions/extensions.css 7 Jun 2005 16:59:54 -0000 @@ -77,6 +77,13 @@ outline: 1px dotted invert ! important; } +richlistbox { + width: 20em; + margin: 10px 10px 5px 10px; + -moz-appearance: listbox; + background-color: Window; +} + why are we setting a width here? this should basically flex to fill, and default width would be controlled by the window Also, you know what I think about random whitespace changes, but some of them were evil, so I'll let it slide ;)
Attachment #185578 - Flags: review?(mconnor) → review-
> Index: toolkit/mozapps/downloads/content/downloads.js > =================================================================== > RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v > retrieving revision 1.43 > diff -u -r1.43 downloads.js > --- toolkit/mozapps/downloads/content/downloads.js 12 Apr 2005 15:55:49 > -0000 1.43 > +++ toolkit/mozapps/downloads/content/downloads.js 7 Jun 2005 16:59:50 > -0000 > @@ -220,6 +220,9 @@ > // Add this download to the percentage average tally > var dl = aSubject.QueryInterface(Components.interfaces.nsIDownload); > gActiveDownloads.push(dl); > + // clear the richlistitem > + gDownloadsView.clearSelection(); > + gDownloadsView.focus(); > > why? > > break; > case "xpinstall-download-started": > var windowArgs = > aSubject.QueryInterface(Components.interfaces.nsISupportsArray); > @@ -261,6 +264,10 @@ > f.remove(false); > > gDownloadViewController.onCommandUpdate(); > + > + // clear the richlistitem > + gDownloadsView.clearSelection(); > + gDownloadsView.focus(); > > why? > > } > > function onDownloadPause(aEvent) > @@ -269,11 +276,18 @@ > gDownloadManager.pauseDownload(uri); > setRDFProperty(uri, "DownloadStatus", > aEvent.target.getAttribute("status-internal")); > setRDFProperty(uri, "ProgressPercent", > aEvent.target.getAttribute("progress")); > + > + // clear the richlistitem > + gDownloadsView.clearSelection(); > + gDownloadsView.focus(); > } > > if we're pausing here, why kill the selection? > > function onDownloadResume(aEvent) > { > gDownloadManager.resumeDownload(aEvent.target.id); > + // clear the richlistitem > + gDownloadsView.clearSelection(); > + gDownloadsView.focus(); > } > > why? > > function onDownloadRemove(aEvent) > @@ -282,6 +296,10 @@ > gDownloadManager.removeDownload(aEvent.target.id); > > gDownloadViewController.onCommandUpdate(); > + > + // clear the richlistitem > + gDownloadsView.clearSelection(); > + gDownloadsView.focus(); > } > } > > is this really what we want to do in this case? I would expect that removing > an item here should follow more typical listbox/file manager behaviour, not > reset focus. > I know that's not what the current dlmgr does, but we need to support this, > here if not > in the binding > The reason for the code is that currently, anytime something is changed in the download manager (an extension manager), the whole list is rebuilt, so focus is lost and has to be reset. Also, the reference held to the current selected element now points to something in memory but not something in the DOM anymore, so it needs to be reset. I believe rob strong has a patch to fix this, so I probably can remove the code.
Attached patch with mconnor's comments (deleted) — — Splinter Review
Attachment #185578 - Attachment is obsolete: true
Attachment #186600 - Flags: review?(mconnor)
Attachment #186600 - Flags: review?(mconnor) → review+
Attachment #186600 - Flags: approval-aviary1.1a2?
Attachment #186600 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Could this have caused bug 298362 (smoketest blocker)?
(In reply to comment #33) > Could this have caused bug 298362 (smoketest blocker)? It did and the patch in bug 298140 that is waiting on review fixes this and a couple of other recent regressions.
Flags: blocking-aviary1.1?
so uh, this broke installing extensions in thunderbird...
(In reply to comment #35) > so uh, this broke installing extensions in thunderbird... There were a couple of regressions that broke things in Firefox and Thunderbird but most of them should be fixed now. bug 298186 bug 298140 - also fixes bug 298362 bug 298164 - should not break functionality
Please file bugs on me if Tbird is still broke.
This patch broke the extension manager dialog in thunderbird. See Bug 285584 for the regression
err Bug 298949 that is
Blocks: 298949
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: