350615 - Subscription options on feed preview page are not accessible

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect, P2)

Description

[Mark Pilgrim (inactive)]

The new inline subscription options on the feed preview page ("Subscribe to this feed using [], Always use X to subscribe to feeds, Subscribe Now) are not exposed to assistive technologies.  They do not expose any accessible objects at all.  I can tab to them, but nothing shows up in Inspect32.

Steps to reproduce in Firefox 2 nightly build 2006-08-29:

1. Preview a feed, such as http://www.mozillazine.org/contents.rdf
2. Tab to document
3. Tab to "Subscribe to this feed using" combo box.  Accessible Event Watcher shows no focus event.  Inspect32 still shows focus on the document.
4. Tab to "Always use Live Bookmarks to subscribe to feeds".  No focus event, focus is still on document.
5. Tab to "Subscribe Now" button.  No focus event, focus is still on document.

This is not merely a focus-related problem.  If you move your cursor over any accessible control, Inspect32 will show you its accessibility properties.  But if you move your cursor over any of these three controls in the header of the feed preview page, Inspect32 shows nothing but the underlying document pane.  These controls simply do not exist in the accessibility hierarchy.

Comment 1

[Robert Sayre]

This is pretty serious. It should block.

Comment 2

[Asaf Romano (gone)]

Note: this is probably branch-only.

Comment 3

[Asaf Romano (gone)]

Attachment: checkpoint

There are some fundamental issues (all look core'ish) here, also the menulist isn't handled at all yet.

Passing this back to mark as I have other stuff on my plate and as he has a windows-eyes copy which doesn't force you to restart after ten minutes of use...

Comment 4

[Aaron Leventhal]

Mano, the checkbox problem was on our end. Thanks for discovering that!

So now it's a matter of getting the combo box working right. Before we work on that, what's your opinion about the keyboard accessibility problem it has. As you arrow down through it (when it's not open), it should not suddenly open a dialog as it does. Would you plan on fixing that by switching to another widget, or by delaying that dialog until the Subscribe button is pressed, or by some other means?

Comment 5

[Aaron Leventhal]

Back to Mano until combobox issue is at least settled (bug 351263).

Mark is on vacation until 9/11.

Comment 6

[Asaf Romano (gone)]

Attachment: almost there

Comment 7

[Asaf Romano (gone)]

Attachment: patch

Comment 8

[Asaf Romano (gone)]

Attachment: patch

Comment 9

[Asaf Romano (gone)]

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

Mike: this is branch only. On trunk, remote xul a11y just works.

Comment 10

[Mike Connor [:mconnor]]

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

Tossing to Dietrich to do a first pass.

Comment 11

[Dietrich Ayala (:dietrich)]

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


Two things I noticed after building w/ this patch on Mac:

1. The combo box is empty except for the live bookmark option, and feed content no longer displays at all. Nothing significant in js console. I pulled a fresh tree recently, so it shouldn't be a local problem... but I'd be glad to be proved wrong here! :)

2. I'm not yet sure if it's related to this patch, but the first time after the page finishes loading, I can't tab to the combo box. I can shift-tab back up to it, and it works thereafter. Just not the first time post-load.

>Index: toolkit/content/widgets/checkbox.xml
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/widgets/checkbox.xml,v
>retrieving revision 1.7.8.4
>diff -u -p -8 -r1.7.8.4 checkbox.xml
>--- toolkit/content/widgets/checkbox.xml	14 Mar 2006 17:05:29 -0000	1.7.8.4
>+++ toolkit/content/widgets/checkbox.xml	5 Sep 2006 03:51:49 -0000
>@@ -19,31 +19,37 @@
>         <xul:label class="checkbox-label" xbl:inherits="xbl:text=label,accesskey,crop" flex="1"/>
>       </xul:hbox>
>     </content>
>     
>     <implementation implements="nsIDOMXULCheckboxElement, nsIAccessibleProvider">
>       <property name="accessible">
>         <getter>
>           <![CDATA[
>-            var accService = Components.classes["@mozilla.org/accessibilityService;1"].getService(Components.interfaces.nsIAccessibilityService);
>-            return accService.createXULCheckboxAccessible(this);
>+            try {
>+              var accService = Components.classes["@mozilla.org/accessibilityService;1"]
>+                                         .getService(Components.interfaces.nsIAccessibilityService);
>+              return accService.createXULCheckboxAccessible(this);
>+            }
>+            catch(ex) { /* no chrome prvilages */ }
                                       ^^^^^^^^^

nit: should be "privileges", here and in several other places in the patch.

>+
>+            return null;
>           ]]>
>         </getter>
>       </property>
> 
>       <method name="setChecked">
>         <parameter name="aValue"/>
>         <body>
>         <![CDATA[
>-          var change = (aValue != (this.getAttribute('checked') == 'true'));
>+          var change = (aValue != (this.getAttributeNS('', 'checked') == 'true'));
>           if (aValue)
>-            this.setAttribute('checked', 'true');
>+            this.setAttributeNS("", 'checked', 'true');
>           else
>-            this.removeAttribute('checked');
>+            this.removeAttributeNS("", 'checked');

nit: 2 instances of mixed quotes, as sayrer pointed out

Comment 12

[Asaf Romano (gone)]

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

Please file a separate bug on the focus issue, I don't think it's caused by this patch (and i'm also pretty-sure this is mac-specifc).

Asking for review again, now that we know why this was broken in your build (flat chrome).

Comment 13

[Dietrich Ayala (:dietrich)]

(In reply to comment #12)
> (From update of attachment 236770 [details] [diff] [review] [edit])
> Please file a separate bug on the focus issue, I don't think it's caused by
> this patch (and i'm also pretty-sure this is mac-specifc).

filed bug 351728

and i filed a dupe for the flat chrome issue, grr :P

Comment 14

[Dietrich Ayala (:dietrich)]

> 
> and i filed a dupe for the flat chrome issue, grr :P

for reference: bug 343360 is the original bug, and bug 351726 was the dupe.

Comment 15

[Dietrich Ayala (:dietrich)]

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

first r=me, with nits mentioned above fixed.

Comment 16

[Mike Connor [:mconnor]]

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

Index: toolkit/content/widgets/text.xml

@@ -361,10 +367,9 @@
 
-</bindings>
-
+</bindings>
\ No newline at end of file

bah!

otherwise, r=me, thanks

Comment 17

[Asaf Romano (gone)]

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

Drivers: this is a branch-only patch.

Comment 18

[Mike Connor [:mconnor]]

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

a=mconnor on behalf of drivers as this is branch only and needs to be landed as soon as possible.

Comment 19

[Asaf Romano (gone)]

Attachment: Updated branch patch (checked in)

with cvs up and comments addressed.

1.8 branch:
mozilla/toolkit/content/widgets/button.xml 1.12.4.5
mozilla/toolkit/content/widgets/checkbox.xml 1.7.8.5
mozilla/toolkit/content/widgets/general.xml 1.11.8.6
mozilla/toolkit/content/widgets/menu.xml 1.9.2.4
mozilla/toolkit/content/widgets/menulist.xml 1.19.8.4
mozilla/toolkit/content/widgets/popup.xml 1.4.52.3
mozilla/toolkit/content/widgets/text.xml 1.18.2.7
mozilla/browser/components/feeds/content/subscribe.xhtml 1.1.2.13
mozilla/browser/components/feeds/src/FeedWriter.js 1.2.2.19

Comment 20

[Asaf Romano (gone)]

Fixed, I'll file a separate bug on porting some of these changes to trunk.