Closed Bug 244624 Opened 20 years ago Closed 20 years ago

XUL:textbox and XUL:menulist not properly exposed

Categories

(Core :: Disability Access APIs, enhancement)

Product:
Core
Component:
Disability Access APIs
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

Details

(Keywords: access)

Attachments

(1 file, 2 obsolete files)

New patch based on Neil's comments. Also changes textbox so it is not an nsIDOMXULLabeledControlElement.
27.34 KB, patch
neil
: review+
alecf
: superreview+
Details | Diff | Splinter Review 
When looking at XUL textboxes or combo boxes (such as those in prefs) in an
accessibility testing tool or with a screen reader, it's clear there are some
problems.

1. Labels are not exposed as the name
2. The readonly flag is not cleared for editable combo boxes
3. Autocomplete textboxes that have a drop down button are really combo boxes
4. There should be some event fired as the text changes because the user arrowed
through the auto complete list
Attachment #149281 - Flags: review?(Louie.Zhao)
Comment on attachment 149281 [details] [diff] [review]
A number of changes were required. Comments in the added code .

>+++ toolkit/content/widgets/autocomplete.xml	25 May 2004 17:00:39 -0000
>@@ -92,9 +89,12 @@
>       <property name="accessible">
>         <getter>
>           <![CDATA[
>+            // When disablehistory="false" then the dropmarker shows up, which makes it a combobox.
>+            // When there's no dropmarker, it's a textfield.
>             var accService = Components.classes["@mozilla.org/accessibilityService;1"].

fix this comment to reference the correct property and r=me on the Fx-specific
changes, pending of course on the overall r/sr comments
Thanks Mike. I have the comments fixed on my local copy. Will post a new patch
if requested to do so. 

Still seeking r=Louie for accessiblity changes.
Seeking r= from Neil on just the xpfe parts.
Comment on attachment 149281 [details] [diff] [review]
A number of changes were required. Comments in the added code .

>+  // If a child is selected, our value is the label of the child.
>+  nsCOMPtr<nsIDOMXULSelectControlElement> select(do_QueryInterface(mDOMNode));
What's wrong with select->GetValue(_retval); rather than the reams of code you
have here?

>+            // When disablehistory="false" then the dropmarker shows up, which makes it a combobox.
>+            // When there's no dropmarker, it's a textfield.
IIRC you'll find that disablehistory="true" hides the dropmarker, making it a
textfield.

>+      <!-- If menu item has no value, return the label as the value -->
Would you mind explaining why?

this.hasAttribute('value') ? this.getAttribute('value') :
this.getAttribute('label')
Is better expressed as this.getAttribute('value') || this.getAttribute('label)

>+            // Use itemlabel to store label of currently selected item. 
>+            // We no longer use label attribute, which would conflict with the 
>+            // actual label for the menulist itself.
Could you explain this too please. Have you actually checked that nobody
depends on the value of the label attribute?

>-    nsIDOMXULTextboxElement
>+    nsIDOMXULTextBoxElement
Why the case change?
(In reply to comment #5)
> (From update of attachment 149281 [details] [diff] [review])
> >+  // If a child is selected, our value is the label of the child.
> >+  nsCOMPtr<nsIDOMXULSelectControlElement> select(do_QueryInterface(mDOMNode));
> What's wrong with select->GetValue(_retval); rather than the reams of code you
> have here?

That's not the correct value for MSAA or ATK. For that we need what's currently
visible in the combobox textfield. The value on nsIDOMXULSelectControlElement
could be something totally unrelated that's never displayed.


> 
> >+            // When disablehistory="false" then the dropmarker shows up,
which makes it a combobox.
> >+            // When there's no dropmarker, it's a textfield.
> IIRC you'll find that disablehistory="true" hides the dropmarker, making it a
> textfield.
disablehistory="true" is the default behavior as well. You have to actually set
it to false if you don't want the drop marker. See pref-navigator.xul
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/content/pref-navigator.xul#103

> 
> >+      <!-- If menu item has no value, return the label as the value -->
> Would you mind explaining why?
I now realize that I don't need this change anymore. Removing from patch.

> 
> this.hasAttribute('value') ? this.getAttribute('value') :
> this.getAttribute('label')
> Is better expressed as this.getAttribute('value') || this.getAttribute('label)
> 
Okay

> >+            // Use itemlabel to store label of currently selected item. 
> >+            // We no longer use label attribute, which would conflict with the 
> >+            // actual label for the menulist itself.
> Could you explain this too please. Have you actually checked that nobody
> depends on the value of the label attribute?
We do depend on the label attribute, that's the problem here. We depend on it to
be what a label is supposed to really mean. The label of a widget is supposed to
be the thing you click on to focus it. It's also the label that's spoken by a
screen reader when you focus the widget so you know which checkbox or combobox
etc. you're on. Whoever wrote this code is using
nsIDOMLabeledControlElement::label in a different way than we use it in other
widgets. It's just wrong the way it is. 
I've checked menulist.xml and nothing else uses the label on the anonymous
xul:label, so it should be safe.
> 
> >-    nsIDOMXULTextboxElement
> >+    nsIDOMXULTextBoxElement
> Why the case change?
> 
We're inconsistent in how we capitalize it and that needed to be fixed.
http://lxr.mozilla.org/seamonkey/search?string=nsidomxultextboxelement

Let me know if you want me to post a new patch with the menu.xml changes removed.

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 149281 [details] [diff] [review])
> > >+  // If a child is selected, our value is the label of the child.
> > >+  nsCOMPtr<nsIDOMXULSelectControlElement> select(do_QueryInterface(mDOMNode));
> > What's wrong with select->GetValue(_retval); rather than the reams of
> > code you have here?
> That's not the correct value for MSAA or ATK. For that we
> need what's currently visible in the combobox textfield.
> The value on nsIDOMXULSelectControlElement could be
> something totally unrelated that's never displayed.
Oh sorry, this is what I really mean:
nsCOMPtr<nsIDOMXULMenuListElement> menulist(do_QueryInterface(mDOMNode));
return menulist->GetLabel(_retval);

> disablehistory="true" is the default behavior as well. You have to
> actually set it to false if you don't want the drop marker.
Only because the code sets it to true if you left it blank...

> Whoever wrote this code is using nsIDOMLabeledControlElement::label
> in a different way than we use it in other widgets.
But a menulist isn't a labeled control element...

> I've checked menulist.xml and nothing else uses the label on the anonymous
> xul:label, so it should be safe.
Actually it was the label attribute on the menulist I was worried about...

> > >-    nsIDOMXULTextboxElement
> > >+    nsIDOMXULTextBoxElement
> > Why the case change?
> > 
> We're inconsistent in how we capitalize it and that needed to be fixed.
> http://lxr.mozilla.org/seamonkey/search?string=nsidomxultextboxelement
Wow, and nobody noticed before?!
http://www.xulplanet.com/references/elemref/ref_menulist.html#attr_accesskey
The label for the menulist should be the thing the accesskey is set on:

For example under prefs -> fonts:
Display _r_esolution: [ 96 dpi (v) ]

The label is also the "name" of a control that gets spoken by a screen reader
when it gets focused.

This is consistent with how we use the term label for other controls like
checkboxes.
Severity: normal → enhancement

        Attachment #149281 -
        Attachment is obsolete: true

    
  

        Attachment #149281 -
        Flags: review?(Louie.Zhao)

    
    

    
  
Comment on attachment 149376 [details] [diff] [review]
New patch based on Neil's comments

Seeking r=louie for stuff in mozilla/accessible

Still seeking r=Neil for the rest.

        Attachment #149376 -
        Flags: review?(Louie.Zhao)

    
    

    
  
Comment on attachment 149376 [details] [diff] [review]
New patch based on Neil's comments

This is looking a lot better! I'm reduced to picking nits ;-)

>+  // If a child is selected, our value is the label of the child.
This comment is inaccurate.

>+  nsCOMPtr<nsIDOMXULMenuListElement> menuList(do_QueryInterface(mDOMNode));
>+  if (menuList) {
>+    return menuList->GetLabel(_retval);
>+  }
>+
>+  // When we're an editable combobox or autocomplete textfield,
>+  // our value is in the text field.
Except that we already found the value, because both editable comboboxes and
autocomplete widgets are menulists. So I'm guessing that you don't need this
fragment:

>+  nsCOMPtr<nsIDOMXULTextBoxElement> textBox(do_QueryInterface(mDOMNode));
>+  if (textBox) {
>+    return textBox->GetValue(_retval);
>+  }

>-      <property name="label" onset="this.setAttribute('label',val); return val;"
>-                             onget="return this.getAttribute('label');"/>
>+      <property name="label" onget="return this.mInputElt.value;"/>
Good catch! Could do with readonly="true" though. (As could the two label
properties in menulist.xml).

>             if (arr && arr.item(0))
>               this.selectedItem = arr[0];
>-            else
>-              this.setAttribute('value', val);
>+            this.setAttribute('value', val);
The selectedItem setter already sets the attribute so technically there's no
point setting it again...

>-            this.setAttribute('value', val.getAttribute('value'));
>+            this.setAttribute('value', val.value);
I'm not sure that this change is worthwhile...

    
  

        Attachment #149376 -
        Flags: review?(Louie.Zhao)

    
  

        Attachment #149406 -
        Attachment description: New patch based on Neil's comments. Also → New patch based on Neil's comments. Also changes textbox so it is not an nsIDOMXULLabeledControlElement.

        Attachment #149406 -
        Flags: review?(neil.parkwaycc.co.uk)

    
    

    
  
Comment on attachment 149406 [details] [diff] [review]
New patch based on Neil's comments. Also changes textbox so it is not an nsIDOMXULLabeledControlElement.

The bits I understand look good to me.

        Attachment #149406 -
        Flags: review?(neil.parkwaycc.co.uk) → review+

    
  

        Attachment #149406 -
        Flags: superreview?(bugs)

    
    

    
  
Comment on attachment 149406 [details] [diff] [review]
New patch based on Neil's comments. Also changes textbox so it is not an nsIDOMXULLabeledControlElement.

Mconner said on IRC I can carry his r= for the firefox changes.

        Attachment #149406 -
        Flags: superreview?(bugs) → superreview?(alecf)

    
    

    
  
Comment on attachment 149406 [details] [diff] [review]
New patch based on Neil's comments. Also changes textbox so it is not an nsIDOMXULLabeledControlElement.

seems reasonable. sr=alecf

        Attachment #149406 -
        Flags: superreview?(alecf) → superreview+

    
    

    
  
Checking in accessible/src/base/nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <-- 
nsAccessibilityService.cpp
new revision: 1.108; previous revision: 1.107
done
Checking in accessible/src/base/nsAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.99; previous revision: 1.98
done
Checking in accessible/src/base/nsAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsAccessible.h,v  <--  nsAccessible.h
new revision: 1.46; previous revision: 1.45
done
Checking in accessible/src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <-- 
nsRootAccessible.cpp
new revision: 1.88; previous revision: 1.87
done
Checking in accessible/src/html/nsHTMLFormControlAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLFormControlAccessible.cpp,v  <-- 
nsHTMLFormControlAccessible.cpp
new revision: 1.50; previous revision: 1.49
done
Checking in accessible/src/xul/nsXULFormControlAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULFormControlAccessible.cpp,v  <-- 
nsXULFormControlAccessible.cpp
new revision: 1.34; previous revision: 1.33
done
Checking in accessible/src/xul/nsXULFormControlAccessible.h;
/cvsroot/mozilla/accessible/src/xul/nsXULFormControlAccessible.h,v  <-- 
nsXULFormControlAccessible.h
new revision: 1.19; previous revision: 1.18
done
Checking in accessible/src/xul/nsXULSelectAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULSelectAccessible.cpp,v  <-- 
nsXULSelectAccessible.cpp
new revision: 1.17; previous revision: 1.16
done
Checking in accessible/src/xul/nsXULSelectAccessible.h;
/cvsroot/mozilla/accessible/src/xul/nsXULSelectAccessible.h,v  <-- 
nsXULSelectAccessible.h
new revision: 1.15; previous revision: 1.14
done
Checking in xpfe/components/autocomplete/resources/content/autocomplete.xml;
/cvsroot/mozilla/xpfe/components/autocomplete/resources/content/autocomplete.xml,v
 <--  autocomplete.xml
new revision: 1.109; previous revision: 1.108
done
Checking in xpfe/global/resources/content/bindings/textbox.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/textbox.xml,v  <-- 
textbox.xml
new revision: 1.27; previous revision: 1.26
done
Checking in xpfe/global/resources/content/bindings/menulist.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/menulist.xml,v  <-- 
menulist.xml
new revision: 1.32; previous revision: 1.31
done
Checking in dom/public/idl/xul/nsIDOMXULMenuListElement.idl;
/cvsroot/mozilla/dom/public/idl/xul/nsIDOMXULMenuListElement.idl,v  <-- 
nsIDOMXULMenuListElement.idl
new revision: 1.3; previous revision: 1.2
done
Checking in dom/public/idl/xul/nsIDOMXULTextboxElement.idl;
/cvsroot/mozilla/dom/public/idl/xul/nsIDOMXULTextboxElement.idl,v  <-- 
nsIDOMXULTextboxElement.idl
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/content/widgets/menulist.xml;
/cvsroot/mozilla/toolkit/content/widgets/menulist.xml,v  <--  menulist.xml
new revision: 1.7; previous revision: 1.6
done
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED

    
    

    
  
Did this cause bug 245325?

    
  
Keywords: aviary-landing

    
    

    
  
Relanding relevant parts of patch following aviary branch landing
Keywords: aviary-landing

    
    

    
  
May I ask why the label property has been made readonly (and its setter removed)?

We want to set the label at least in two places. This is the code on the aviary
branch:
http://lxr.mozilla.org/aviarybranch/source/browser/components/prefwindow/content/pref-languages.js#267
http://lxr.mozilla.org/aviarybranch/source/browser/components/bookmarks/content/addBookmark2.js#245

I fixed the first example by using .setAttribute("label", newlabel):
http://lxr.mozilla.org/seamonkey/source/browser/components/prefwindow/content/pref-languages.js#267
This is still used in Ben's preferences rewrite:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/preferences/languages.js&rev=PREFERENCES_20050101_BRANCH#177

I just noticed that the second example has been fixed by using 5 (!) lines of
code (bug 275405):
http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/content/addBookmark2.js#245

    
    

    
  
>http://lxr.mozilla.org/aviarybranch/source/browser/components/bookmarks/content/addBookmark2.js#245
> Please read bug 275405 comment 3.

In that case, the item that needed to be displayed is not actually in the
menulist, so it was necesary to add it for it to be displayed. Previously, we
only set the label to achieve the same effect without having to actually add the
item to the list.

    
    

    
  
(In reply to comment #21)
>the item that needed to be displayed is not actually in the menulist
I don't know what this particular list does but I was wondering why you had a
list where you couldn't actually select the "default" item.

    
    

    
  
>http://lxr.mozilla.org/aviarybranch/source/browser/components/prefwindow/content/pref-languages.js#267
> Please use the appropriate widget. See SearchDialog.xul for an example.
I don't understand. Do you mean the folder picker in the Search Messages dialog?
That one is a menulist as well:
http://lxr.mozilla.org/seamonkey/source/mail/base/content/SearchDialog.xul#91

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/search/resources/content/SearchDialog.js&mark=301,351-361#299
  calls SetFolderPicker, which sets the label in a familiar way:  
http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/msgFolderPickerOverlay.js#119
  picker.setAttribute("label",selectedValue);

So this is the third instance I know of (without having searched further) where
a setter for the label property of a menulist would be useful.

> >the item that needed to be displayed is not actually in the menulist
> I don't know what this particular list does but I was wondering why you had a
> list where you couldn't actually select the "default" item.
Please have a look at Firefox's Add Bookmark dialog. The "Create In" menulist
shows the 5 last used folders, plus the bookmarks root folder on top of them. If
you click the down arrow, a tree is displayed containing all bookmarks folders.
If you select a folder in the tree, its name is supposed to be shown in the
"Create In" menulist above as well. This was done previously by setting the
label of the menulist.

    
    

    
  
Oh, and there's a bug filed to convert those folder pickers to better widgets.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: