Closed Bug 407359 (mainscreena11y) Opened 17 years ago Closed 16 years ago

Popups on main screen have issues

Categories

(Core :: Disability Access APIs, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta3

People

(Reporter: MarcoZ, Assigned: aaronlev)

References

(Depends on 1 open bug)

Details

(Keywords: access)

Attachments

(5 files, 13 obsolete files)

30.80 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
31.59 KB, patch
MarcoZ
: review+
enndeakin
: review+
Details | Diff | Splinter Review
47.14 KB, patch
surkov
: review+
Details | Diff | Splinter Review
43.27 KB, patch
asaf
: review+
enndeakin
: review+
mconnor
: ui-review+
Details | Diff | Splinter Review
979 bytes, patch
asaf
: review+
Details | Diff | Splinter Review
Currently, the new AutoComplete popup container is being exposed as a ROLE_POPUP, which makes it appear like a menu to screen readers. Some of them have problems coping with contained controls such as the RichListbox within it. This results in them not reading items at all, or only reading parts of them.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Marco, please test with JAWS, Window-Eyes and Orca.

Reviewer, this is what the patch does:
1) Refine code that determines whether something is a combo box list or not
2) Fix multiselect state detection tree (for search bar autocomplete results, which was saying "multiselectable")
3) Map rich list option to list item (not list!). JAWS was saying "list" as each in rich list was focused
4) Use different bindings for autocomplete popup, menupopup and panel, to ensure a menupopup is exposed as a real menu, but a results list is just a list, and any other popup panel is exposed as a groupbox, otherwise we expose menupopup events for it and Window-Eyes gets confused in bookmarks dialog
5) Some buttons in the toolbar use the xul:image element, and one uses the xul:box. However, they are still buttons
6) For the <box id="identity-box">, which is really a button, put the tooltiptext on it instead of the children, so the accessible name is correct
Blocks: 393398
Generalizing the bug. This patch also fixes some problems with the Larry popup and the bookmark popup.

There is a separate bug to make Larry part of the tab order -- bug 400888.
No longer blocks: 393398
Summary: Don't expose AutoComplete popup as being a menu to ATs → Popups on main screen have issues
Blocks: 393988
Blocks: 400888
First issue: When returning focus to the Location bar from the search results, either by hitting Escape or RightArrow, no focus event is fired for the Location bar. Neither JAWS nor Window-Eyes know that focus has changed back to Location bar.
(In reply to comment #3)
> First issue: When returning focus to the Location bar from the search results,
> either by hitting Escape or RightArrow, no focus event is fired for the
> Location bar. Neither JAWS nor Window-Eyes know that focus has changed back to
> Location bar.

Same goes for Orca on Linux.

Additionally:
1. When hitting CTRL+D to add a bookmark, none of the products notice the containing panel. JAWS and Orca simply speak the names like "name", "location", "tags" etc. along with their respective types. This was true for JAWS and Orca previously, too, but now without the "menu" stuff. 

2. Window-Eyes still has trouble with that Add Bookmarks panel: After pressing CTRL+D, the first textbox "Name" is read, but tabbing through the panel subsequently does not yield any speech whatsoever. WE only returns to speaking after hitting ESCAPE and TAB to put focus on the Location bar.

3. Downloads window still works with all products. Orca might need an adjustment because the ListItems have been converted from being a List to being a ListItem. Orca does not announce "1 of x" for these items at all, but it speaks the focused item reliably.
Attached patch See comments about improvements (obsolete) — Splinter Review
Marco, can you test?

Improvements: 
1) Initial focus on link for identity panel if opened with keyboard
2) Restore focus after a panel closes
3) Show focus outline on identity box button when it has focus
4) Accessible name for bookmarks panel (requires fix from bug 398910 to work)

I'm not sure what problem Window-Eyes has with the bookmarks panel but it might require a bug fix to allow controls in popups from their end. Since they already don't work with that dialog I think we're best focusing on JAWS and Orca first, and then providing them with the nightly build to address their issues next.

I'm not sure about the focus appearance on Mac, whether this works.
Attachment #292255 - Attachment is obsolete: true
Some quick feedback before I leave for my Mom's birthday:
1. Still no focus event when returning to Location bar.
2. There is now an item when shift+tabbing from the Location bar that currently has no accessible name and role. Pressing SPACE gives a button "Tell me more about this page.", but no indication of that this is actually part of a panel with information. I suppose this is the Larry UI?
Adding relnote keyword 
Keywords: relnote
I didn't get about foucs in location bar. When I navigate through URL items of the popup of location bar then focus is always on textbox of location bar. What focus issue you talk about?
in the patch you make xul:popup unaccessible, though it's deprecated but how can popup be used?
(In reply to comment #9)
> in the patch you make xul:popup unaccessible, though it's deprecated but how
> can popup be used?
> 

(In reply to comment #8)
> I didn't get about foucs in location bar. When I navigate through URL items of
> the popup of location bar then focus is always on textbox of location bar. What
> focus issue you talk about?

When I navigate through items that are in the popup, I get focus events for each Item I highlight. However, once an item is highlighted, you can press RightArrow to take that item's URL into the location bar. Now, the popup goes away, and focus should return to the location bar Edit Combo. Visually, itdoes, but there is no focus event generated for this particular instance.
Surkov, Marco -- which tools are you each using to test?
MSAA tools, JAWS, a DOM focus listener? ...

popupstart/end events could also affect what JAWS considers to be focused.
(In reply to comment #11)
> Surkov, Marco -- which tools are you each using to test?
> MSAA tools, JAWS, a DOM focus listener? ...

JAWS and AccEvent32. And on Linux: Orca.

> popupstart/end events could also affect what JAWS considers to be focused.

AFAICS, there are no POPUPSTART or POPUPEND events any longer. JAWS does no longer consider anything to be a menu with your patch, which is good. It just does not announce anything when focus leaves the autoComplete list entries and goes back to the Location bar. I can follow the focus change with my braille display. Same goes for Orca, it simply keeps silent once focus returns to Location bar.
You're right, there's no focus event if I hit right arrow from the autocomplete. But, there is one if I hit Escape! Yikes!
Actually, duh, that's in current builds that right arrow doesn't return focus to the right place.
Alias: mainscreena11y
Attached patch New patch for Marco to try (obsolete) — Splinter Review
1) Focus not going back to location bar, fixed
2) Shift+tab not reporting the widget, fixed
3) Pressing space to open identity popup, only link read -- please try again. We fire an EVENT_ALERT for the popup followed quickly by a FOCUS event to the link with the focus. Either the focus event is making JAWS forget the alert event or they aren't processing the ALERT_EVENT how we'd like. Also please try with other screen readers that process alerts -- Window-Eyes and Orca.
Attachment #292489 - Attachment is obsolete: true
First of all: This patch breaks context menus on all platforms and with all screen readers. For example, when pressing Shift+F10 in the Search edit of Bugzilla, none of the screen readers will announce the fact that the context menu comes up. Orca also won't read any of the focused items, JAWS will read the selected item, but it will neither speak the ContextMenu, nor the Leaving Menus messages.

Below are the things that do work or can be reported at:
(In reply to comment #15)
> 1) Focus not going back to location bar, fixed
> 2) Shift+tab not reporting the widget, fixed

Both confirmed.

> 3) Pressing space to open identity popup, only link read -- please try again.
> We fire an EVENT_ALERT for the popup followed quickly by a FOCUS event to the
> link with the focus. Either the focus event is making JAWS forget the alert
> event or they aren't processing the ALERT_EVENT how we'd like. Also please try
> with other screen readers that process alerts -- Window-Eyes and Orca.

JAWS 8.0 still only reports the button. 
JAWS 9 reads the complete alert and the button, but repeats each line twice. 
Window-Eyes stays completely silent, it neither speaks the alert nor the button. When I ask it for the current element, it only speaks "button", but not the button's label.
Orca also speaks the button, and when pressing SPACE, it will only speak the "Tell me more about this website..." button, but not the alert.

I'm also noticing that you can't type very fast in the autocomplete otherwise it selects all the text and you end up writing over yourself. I think I know what's causing that.

The alert problems are probably something to follow up with the individual screen reader vendors on.

Are you having those shift+10 problems everywhere? For example, in the context menu for a link in a web page?
(In reply to comment #18)
> Are you having those shift+10 problems everywhere? For example, in the context
> menu for a link in a web page?

Yes, all acrosss the board, completely broken no matter where you right-click/Shift+F10.
Attached patch Address Marco's issues (obsolete) — Splinter Review
Attachment #293888 - Attachment is obsolete: true
Is bug 409148 a dupe of this? It deals with the new autocomplete popup but is about Tablet PC input
Attachment #293981 - Attachment is obsolete: true
(In reply to comment #22)
> Avoid extra focus event when typing in URL bar

Well, the AutoComplete has reverted to being a menu. Not good for WE.
Add Bookmarks, however, is not a menu, which is good.

Alert is now spoken also with JAWS 8.0.

Waiting on further testing until AutoComplete is resolved.
Attachment #294030 - Flags: superreview?(mano)
Attachment #294030 - Flags: review?(surkov.alexander)
Comment on attachment 294030 [details] [diff] [review]
Avoid extra focus event when typing in URL bar

Enn should review this.
Attachment #294030 - Flags: superreview?(mano)
Attachment #294030 - Attachment is obsolete: true
Attachment #294030 - Flags: review?(surkov.alexander)
Attachment #294240 - Flags: superreview?
Attachment #294240 - Flags: review?(surkov.alexander)
Attachment #294240 - Flags: superreview?
Attachment #294240 - Flags: review?(enndeakin)
Comment on attachment 294240 [details] [diff] [review]
1) Identity popup gets spoken by JAWS now, 2) Expose all xul links as links (not buttons)

> Index: browser/base/content/browser.js
> +  focusById: function(id) {
> +    document.getElementById(id).focus();
> +  },
> +  
> +  hideIdentityPopup : function() {
> +    this._identityPopup.hidden = true;

Do you mean to hide a popup here with hidePopup?

Index: toolkit/content/widgets/autocomplete.xml

> -      <handler event="popuphiding"><![CDATA[
> +      <handler event="popuphiding">
> +          <![CDATA[
>          var controller = this.view.QueryInterface(Components.interfaces.nsIAutoCompleteController);

Fix up identation

>          // see bug #400671 for details
> +        var isListFocused = true;
> +        if (this.selectedIndex == -1)
> +          isListFocused = false;
>          this.selectedIndex = -1;
>          this.mPopupOpen = false;
>
>          // Reset the maxRows property to the cached "normal" value, and reset
>          // _normalMaxRows so that we can detect whether it was set by the input
>          // when the popupshowing handler runs.
>          this.mInput.maxRows = this._normalMaxRows;
>          this._normalMaxRows = -1;
> +        if (isListFocused)
> +          this.mInput._focus();
>        ]]></handler>

Can you explain what these autocomplete.xml changes do?

Index: toolkit/content/widgets/popup.xml
 
> +  <binding id="group-panel"
> +           extends="chrome://global/content/bindings/popup.xml#panel">

Why this separate binding?

> +    <implementation implements="nsIDOMXULPopupElement, nsIAccessibleProvider">
> +      <property name="accessibleType" readonly="true">
> +        <getter>
> +          <![CDATA[
> +          return (this.getAttribute("noautofocus") == "true") ?
> +                       Components.interfaces.nsIAccessibleProvider.XULAlert :
> +                       Components.interfaces.nsIAccessibleProvider.XULGroupbox;

Huh? Panels aren't groupboxes. Can you explain what this is for?

> +        ]]></getter>
> +      </property>
> +      <field name="_prevFocusedElement">0</field>
> +    </implementation>
> +    
> +    <handlers>
> +      <handler event="popupshowing"><![CDATA[
> +        _prevFocusedElement = document.commandDispatcher.focusedElement;

Use this._prevFocusedElement. Also this can cause an exception in non-privileged documents
(as with the popuphiding handler below).

> +        // Fire event for accessibility APIs
> +        var event = document.createEvent("Events");
> +        event.initEvent("AlertActive", true, true);
> +        this.dispatchEvent(event);

The panel may never actually appear though if someone cancelled the popupshowing event.
I would guess you really want the popupshown event which fires after the popup is shown.

> +      <handler event="popuphiding">
...
> +              while (ancestorOfFocus && ancestorOfFocus != this) {
> +                ancestorOfFocus = ancestorOfFocus.parentNode;
> +              }
> +              if (ancestorOfFocus != aPanel)
> +                return; // Focus was was already set on an element outside of this panel

'this' will refer to the restoreFocus function. You should just remove the second
condition on the while and move the if condition inside the loop.

> +            }
> +            if (_prevFocusedElement)
> +              aPanel._prevFocusedElement.focus();
> +            else
> +              aPanel._prevFocused.contentWindow.focus();

You never set _prevFocused to anything. I'm assuming this else block should be removed.

Is this code the fix for bug 402499?

> +          }
> +          setTimeout(restoreFocus, 0, this);

As with popupshowing, popuphiding fires before the popup will be hidden and it may never
hide if an event handler cancels the event. You want the popuphidden event here, which may
eliminate the need to put this in a timeout callback at all.
Attachment #294240 - Flags: review?(enndeakin) → review-
Attached patch Address Neil's comments (obsolete) — Splinter Review
Attachment #294240 - Attachment is obsolete: true
Attachment #294281 - Flags: review?(enndeakin)
Attachment #294240 - Flags: review?(surkov.alexander)
Attached patch Address Neil's comments (obsolete) — Splinter Review
Attachment #294282 - Flags: review?(enndeakin)
Attachment #294281 - Attachment is obsolete: true
Attachment #294281 - Flags: review?(enndeakin)
[code snipped]
> Do you mean to hide a popup here with hidePopup?
Yes, done


> >          // see bug #400671 for details
> > +        var isListFocused = true;
> > +        if (this.selectedIndex == -1)
> > +          isListFocused = false;
> >          this.selectedIndex = -1;
> >          this.mPopupOpen = false;
> >
> >          // Reset the maxRows property to the cached "normal" value, and reset
> >          // _normalMaxRows so that we can detect whether it was set by the input
> >          // when the popupshowing handler runs.
> >          this.mInput.maxRows = this._normalMaxRows;
> >          this._normalMaxRows = -1;
> > +        if (isListFocused)
> > +          this.mInput._focus();
> >        ]]></handler>
> 
> Can you explain what these autocomplete.xml changes do?
I added a comment to the code. Basically, if there was a list item that was active, we were firing a11y focus events. By refocusing the input we fire a focus event for the input to update assitive tech's idea of where focus is. There is some code to make sure we don't fire it when the input already has focus, since that would cause a redundant event.

 
> Index: toolkit/content/widgets/popup.xml
> 
> > +  <binding id="group-panel"
> > +           extends="chrome://global/content/bindings/popup.xml#panel">
> 
> Why this separate binding?
Because other kinds of popups inherit from panel. We need to change the accessible provider for actual <panel>'s but we don't want the other popups to change their nsIAccessibleProvider impls.

> > +    <implementation implements="nsIDOMXULPopupElement, nsIAccessibleProvider">
> > +      <property name="accessibleType" readonly="true">
> > +        <getter>
> > +          <![CDATA[
> > +          return (this.getAttribute("noautofocus") == "true") ?
> > +                       Components.interfaces.nsIAccessibleProvider.XULAlert :
> > +                       Components.interfaces.nsIAccessibleProvider.XULGroupbox;
> 
> Huh? Panels aren't groupboxes. Can you explain what this is for?
It was admittedly a hack which makes JAWS work. I've changed it to XULPane, which is more sensible. We do the right thing for that in our MSAA layer anyway. If focus doesn't go in there automatically we guess that it should just be automatically read as an alert.

I've fixed up all the popup showing/hiding/hidden stuff. 
> Is this code the fix for bug 402499?
Yes. I didn't know there was a bug filed for it already.
> > 
> > > +  <binding id="group-panel"
> > > +           extends="chrome://global/content/bindings/popup.xml#panel">
> > 
> > Why this separate binding?
> Because other kinds of popups inherit from panel. We need to change the
> accessible provider for actual <panel>'s but we don't want the other popups to
> change their nsIAccessibleProvider impls.

In that case, we should just merge the current id="panel" binding into "popup-base" and call this new binding "panel" instead of "group-panel".

Neil, in that case why do we have a separate "popup-base" binding now?
(In reply to comment #31)
> Neil, in that case why do we have a separate "popup-base" binding now?
> 

I don't think there's a reason, other than perhaps it was needed at one time.
OK, I ran with this patch for a while, and there is only one remaining problem AFAICS:

1. Focus the Location Bar.
2. Type some part of an address or title you know you get a match for.
3. Now, type something that you know will definitely not match.
Result: The moment Firefox recognizes that the entered text no longer matches anything, something happens that causes all text in the text field to be highlighted, as if focus just went there. Continuing to type will, of course, erase everything highlighted.

Other than that:
1. Add Bookmarks now announces itself properly. Great!
2. Alerts are really now spoken once. Fantastic!
3. Transitioning back and forth from the matches list to the Location bar  works fine.

Also tested with WE and it works fine, too, except for the problem with text being highlighted, that's common to both JAWS and WE.
Comment on attachment 295833 [details] [diff] [review]
1) Combine bindings so we can remove group-panel binding, 2) Fix issue with rich list autocomplete being reported as menu (should be list), 3) fix issue with JAWS reading alert text twice

> Index: toolkit/content/widgets/popup.xml
> ===================================================================
> RCS file: /cvsroot/mozilla/toolkit/content/widgets/popup.xml,v
>  
> +      <handler event="popuphiding"><![CDATA[

Was there a reason you didn't change this to popuphidden (last point of comment 26)?

> +            if (ancestorOfFocus == aPanel) {
> +              // Focus was set on an element inside this panel,
> +              // so we need to move it back to where it was previously
> +              dump("\n-noodlehead-\n");

remove this

> +              prevFocus.focus();
> +              return;
> +            }
> +            ancestorOfFocus = ancestorOfFocus.parentNode;
> +          }
> +        }
> +        if (this._prevFocus)
> +          setTimeout(restoreFocusIfInPanel, 0, this,
> +                     document.commandDispatcher.focusedElement,
> +                     this._prevFocus);

All the references to the commandDispatcher should go in a try/catch block. You might also be able to use document.activeElement instead.

> +NS_IMETHODIMP
> +nsXULAlertAccessible::GetName(nsAString& aName)
> +{
> +  aName.Truncate();
> +  return NS_OK;
> +  nsCOMPtr<nsIContent> content(do_QueryInterface(mDOMNode));

I didn't review the accessible part, but I noticed the return here with code after it.

Otherwise, the toolkit part looks ok.
Attachment #295833 - Flags: review?(enndeakin) → review+
> Was there a reason you didn't change this to popuphidden (last point of comment 26)?
Yes -- I tried that and it didn't work. This was a couple of weeks ago already but my recollection is that the event was cancelled in some cases and the code wouldn't run. I'm not 100% sure that's what it was, because it was a couple of weeks ago.

> 
> > +            if (ancestorOfFocus == aPanel) {
> > +              // Focus was set on an element inside this panel,
> > +              // so we need to move it back to where it was previously
> > +              dump("\n-noodlehead-\n");
> 
> remove this
> 
> > +              prevFocus.focus();
> > +              return;
> > +            }
> > +            ancestorOfFocus = ancestorOfFocus.parentNode;
> > +          }
> > +        }
> > +        if (this._prevFocus)
> > +          setTimeout(restoreFocusIfInPanel, 0, this,
> > +                     document.commandDispatcher.focusedElement,
> > +                     this._prevFocus);
> 
> All the references to the commandDispatcher should go in a try/catch block. 
Okay.
> You might also be able to use document.activeElement instead.
I'm not sure which document is active so I think I want to keep using commandDispatcher.focusedElement to get the global focus and add try/catch as you suggest.

Blocks: 402499
Attached patch Address Neil & Marco's comments (obsolete) — Splinter Review
Attachment #295944 - Flags: review?(surkov.alexander)
This one looks good! No random selection of all previously typed text anymore.
Comment on attachment 295944 [details] [diff] [review]
Address Neil & Marco's comments

Seeking r=mano for browser bits.
Attachment #295944 - Flags: review?(mano)
> Index: accessible/src/xul/nsXULTreeAccessible.cpp
> ===================================================================
> @@ -182,11 +182,11 @@ nsXULTreeAccessible::GetState(PRUint32 *
>    nsCOMPtr<nsIDOMElement> element (do_QueryInterface(mDOMNode));
>    if (element) {
>      // the default selection type is multiple
>      nsAutoString selType;
>      element->GetAttribute(NS_LITERAL_STRING("seltype"), selType);
> -    if (selType.IsEmpty() || !selType.EqualsLiteral("single"))
> +    if (selType.EqualsLiteral("multiple"))
>        *aState |= nsIAccessibleStates::STATE_MULTISELECTABLE;
>    }
>  
>    *aState |= nsIAccessibleStates::STATE_READONLY |
>               nsIAccessibleStates::STATE_FOCUSABLE;

Shouldn't the comment be "the default selection type is single"? Because you're setting the multiple state explicitly.
(In reply to comment #42)
> > Index: accessible/src/xul/nsXULTreeAccessible.cpp
> > ===================================================================
> > @@ -182,11 +182,11 @@ nsXULTreeAccessible::GetState(PRUint32 *
> >    nsCOMPtr<nsIDOMElement> element (do_QueryInterface(mDOMNode));
> >    if (element) {
> >      // the default selection type is multiple
> >      nsAutoString selType;
> >      element->GetAttribute(NS_LITERAL_STRING("seltype"), selType);
> > -    if (selType.IsEmpty() || !selType.EqualsLiteral("single"))
> > +    if (selType.EqualsLiteral("multiple"))
> >        *aState |= nsIAccessibleStates::STATE_MULTISELECTABLE;
> 
> Shouldn't the comment be "the default selection type is single"? Because you're
> setting the multiple state explicitly.

I don't understand this change. The default selection type for trees is 'multiple'. The existing code looks correct.

What makes this tree single select? It's the autocomplete results for the search box:
http://mxr.mozilla.org/seamonkey/source/toolkit/content/widgets/autocomplete.xml#529
The reason that the autocomplete tree doesn't support multiple selection is that it does the input handling itself. However, it should say that it's a single selection tree so that a11y APIs know about it.
Attachment #295944 - Attachment is obsolete: true
Attachment #296180 - Flags: review?(surkov.alexander)
Attachment #296180 - Flags: review?(enndeakin)
Attachment #295944 - Flags: review?(surkov.alexander)
Attachment #295944 - Flags: review?(mano)
Attachment #296180 - Flags: review?(mano)
Attachment #296180 - Flags: review?(enndeakin) → review+
Blocks: 412214
Comment on attachment 296180 [details] [diff] [review]
1) Add seltype="single" to autocomplete results xul:tree, 2) Remove bogus change to nsXULTreeAccessible -- default seltype is multiple

r+ by MarcoZ. Surkov, do you agree?
Attachment #296180 - Flags: review?(surkov.alexander) → review+
Attachment #296180 - Flags: review?(surkov.alexander)
Comment on attachment 296180 [details] [diff] [review]
1) Add seltype="single" to autocomplete results xul:tree, 2) Remove bogus change to nsXULTreeAccessible -- default seltype is multiple

I didn't follow yet all discussion in the bug to speed up the review therefore I'm sorry if I ask known issue.

>+    else if (event.type == "keypress") {
>+      if (event.charCode != 32 && event.keyCode != 13)

That would be readable if you will use nsIDOMKeyEvent constants like DOM_VK_SPACE and DOM_VK_RETURN.

>   <binding id="autocomplete-base-popup" extends="chrome://global/content/bindings/popup.xml#popup">
>-    <implementation implements="nsIAutoCompletePopup">
>+    <implementation implements="nsIAutoCompletePopup,nsIAccessibleProvider">
>+      <property name="accessibleType" readonly="true"
>+                onget="return Components.interfaces.nsIAccessibleProvider.XULListbox;"/>

Does it mean
panel[type="autocomplete"] has accessible tree
XULListbox
  XULTree
    XULTreeItem
and
panel[type="rich-autocomplete"] has accessible tree
XULListbox
  XULListbox
    XULListboxItem


>-  if ((mParent && Role(mParent) == nsIAccessibleRole::ROLE_COMBOBOX) ||
>-      content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::type,
>+  if (content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::type,
>                            nsAccessibilityAtoms::autocomplete, eIgnoreCase)) {

Could you give me a hint how it's possible because it sounds autocomplete uses panels but they aren't nsXULMenupopupAccessible if I'm correct.
(In reply to comment #47)
> >-  if ((mParent && Role(mParent) == nsIAccessibleRole::ROLE_COMBOBOX) ||
> >-      content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::type,
> >+  if (content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::type,
> >                            nsAccessibilityAtoms::autocomplete, eIgnoreCase)) {
> 
> Could you give me a hint how it's possible because it sounds autocomplete uses
> panels but they aren't nsXULMenupopupAccessible if I'm correct.

They currently are. So, the AutocompletePopup panel appears, and the screen reader gets a MENUPOPUP event, which confuses several of the screen readers. Same goes for the Add Bookmarks panel. This bug intends to change those to be panels instead of MENUPOPUPs.
Attachment #297377 - Flags: review?(surkov.alexander)
Attachment #297377 - Flags: review?(mano)
(In reply to comment #48)
> (In reply to comment #47)
> > >-  if ((mParent && Role(mParent) == nsIAccessibleRole::ROLE_COMBOBOX) ||
> > >-      content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::type,
> > >+  if (content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::type,
> > >                            nsAccessibilityAtoms::autocomplete, eIgnoreCase)) {
> > 
> > Could you give me a hint how it's possible because it sounds autocomplete uses
> > panels but they aren't nsXULMenupopupAccessible if I'm correct.
> 
> They currently are. So, the AutocompletePopup panel appears, and the screen
> reader gets a MENUPOPUP event, which confuses several of the screen readers.
> Same goes for the Add Bookmarks panel. This bug intends to change those to be
> panels instead of MENUPOPUPs.
> 

Yes but as you said we change those to be panels so why do we need to check "autocomplete" attribute any more in menupopups accessible? What am I missing?
Attachment #297377 - Attachment is obsolete: true
Attachment #297383 - Flags: review?(surkov.alexander)
Attachment #297383 - Flags: review?(mano)
Attachment #297377 - Flags: review?(surkov.alexander)
Attachment #297377 - Flags: review?(mano)
Attachment #296180 - Flags: review?(surkov.alexander)
Attachment #296180 - Flags: review?(mano)
Comment on attachment 297383 [details] [diff] [review]
Surkov is right, we don't need that autocomplete test in nsXULMenuAccessible anymore

Just the browser/ bits (and one nit in toolkit)...

>Index: browser/base/content/browser.js
>===================================================================

>     // Push the appropriate strings out to the UI
>     this._identityPopupContent.textContent = body;
>     this._identityPopupContentSupp.textContent = supplemental;
>     this._identityPopupContentVerif.textContent = verifier;
>   },
>+
>+  focusById: function(id) {
>+    document.getElementById(id).focus();
>+  },

This one is too short and only used once, so just inline it contents (something like setTimeout(function() { ...focus(); }, 0);

>+  

nit: trailing spaces.

>   
>   /**
>    * Click handler for the identity-box element in primary chrome.  
>    */
>-  handleIdentityClick : function(event) {
>+  showIdentityPopup : function(event) {

hrm, maybe handleIdentityButtonEvent?
>     event.stopPropagation();
> 
>-    if (event.button != 0)
>-      return; // We only want left-clicks
>-        
>+    if (event.type == "click") {
>+      if (event.button != 0)
>+        return;  // Left clicks only
>+    }

make that:
  if (event.type == "click" && event.button == 0)
    return;  // Left clicks only


>+    else if (event.type == "keypress") {
>+      if (event.charCode != Components.interfaces.nsIDOMKeyEvent.DOM_VK_SPACE &&
>+          event.keyCode != Components.interfaces.nsIDOMKeyEvent.DOM_VK_RETURN)
>+        return;  // Space or enter only
>+    }
>+

You could use KeyEvent..DOM_VK_SPACE/RETURN;

>     // Make sure that the display:none style we set in xul is removed now that
>     // the popup is actually needed
>     this._identityPopup.hidden = false;
>     
>     // Tell the popup to consume dismiss clicks, to avoid bug 395314
>@@ -5704,10 +5719,15 @@ IdentityHandler.prototype = {
>     // Update the popup strings
>     this.setPopupMessages(this._identityBox.className);
>     
>     // Now open the popup, anchored off the primary chrome element
>     this._identityPopup.openPopup(this._identityBox, 'after_start');
>+    
>+    if (event.type == "keypress") {
>+      // If opened with keyboard, focus the more info link
>+      setTimeout(this.focusById, 0, 'identity-popup-more-info-link');
>+    }

See above. And I think you could use a popupshown handler, rather than setTimeout.


>Index: browser/base/content/browser.xul
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser.xul,v
>retrieving revision 1.402
>diff -p -u -5 -r1.402 browser.xul
>--- browser/base/content/browser.xul	9 Jan 2008 04:04:31 -0000	1.402
>+++ browser/base/content/browser.xul	16 Jan 2008 19:04:18 -0000
>@@ -104,11 +104,13 @@
> 
>     <!-- for url bar autocomplete -->
>     <panel type="autocomplete-richlistbox" chromedir="&locale.dir;" id="PopupAutoCompleteRichResult" noautofocus="true" hidden="true"/>
> 
>     <panel id="editBookmarkPanel" orient="vertical" hidden="true"
>+           aria-labelledby="editBookmarkLabel"
>            

hrm, so a11y/ code doesn't support <panel label=".."/> (label/title)?

onpopupshown="PlacesCommandHook.editBookmarkPanelShown();">
>+      <label hidden="true" id="editBookmarkLabel">&bookmarkPageCmd.label;</label>

This dialog is opened for other commands as well (Bookmark this link/frame), and may soon have visible titles like "Page Bookmarked" and "Edit Bookmark".

>       <vbox id="editBookmarkPanelContent" flex="1"/>
>       <hbox flex="1">
>         <spacer flex="1"/>
>         <button id="editBookmarkPanelDeleteButton"
>                 label="&editBookmark.delete.label;"
>@@ -143,11 +145,12 @@
>     </popup>
> 
>     <popup id="placesContext"/>
> 
>     <!-- Popup for site identity information -->
>-    <panel id="identity-popup" position="after_start" hidden="true" noautofocus="true">
>+    <panel id="identity-popup" position="after_start" hidden="true" noautofocus="true"
>+           onpopuphiding="document.getElementById('urlbar').focus();">

I cannot recall whether there's a way to open Larry if the location bar is hidden, please ask Jhonatan or Gavin.

> 
>@@ -267,24 +271,24 @@
>                  ontextentered="return handleURLBarCommand(param);"
>                  ontextreverted="return handleURLBarRevert();"
>                  pageproxystate="invalid">
>           <!-- Use onclick instead of normal popup= syntax since the popup
>                code fires onmousedown, and hence eats our favicon drag events -->
>-          <box id="identity-box"
>-               onclick="getIdentityHandler().handleIdentityClick(event);">
>+          <box id="identity-box" role="button"
>+               onclick="getIdentityHandler().showIdentityPopup(event);"
>+               onkeypress="getIdentityHandler().showIdentityPopup(event);"
>+               tooltiptext="&proxyIcon.tooltip;">
>             <hbox align="center">
>               <deck id="page-proxy-deck" onclick="PageProxyClickHandler(event);">
>                 <image id="page-proxy-button"
>-                       ondraggesture="PageProxyDragGesture(event);"
>-                       tooltiptext="&proxyIcon.tooltip;"/>
>+                       ondraggesture="PageProxyDragGesture(event);"/>
>                 <image id="page-proxy-favicon" validate="never"
>                        ondraggesture="PageProxyDragGesture(event);"
>                        onload="this.parentNode.selectedIndex = 1;
>                                event.stopPropagation();"
>                        onerror="this.removeAttribute('src');
>-                                this.parentNode.selectedIndex = 0;"
>-                       tooltiptext="&proxyIcon.tooltip;"/>
>+                                this.parentNode.selectedIndex = 0;"/>

what's this all about?


>Index: browser/themes/gnomestripe/browser/browser.css
>===================================================================
>RCS file: /cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v
>retrieving revision 1.151
>diff -p -u -5 -r1.151 browser.css
>--- browser/themes/gnomestripe/browser/browser.css	6 Jan 2008 03:58:04 -0000	1.151
>+++ browser/themes/gnomestripe/browser/browser.css	16 Jan 2008 19:04:20 -0000
>@@ -825,10 +825,15 @@ toolbar[iconsize="small"] #paste-button[
> 
> /* Identity indicator */
> #identity-box {
>   background-color: -moz-dialog;
>   -moz-border-end: 1px solid ThreeDShadow;
>+  -moz-user-focus: normal;
>+}
>+
>+#identity-box:focus {
>+  outline: 1px dotted -moz-DialogText;
> }
> 

It would be cleaner to make this a xul:button (probably with -moz-appearance:none)


>Index: browser/themes/pinstripe/browser/browser.css
>===================================================================
>RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v
>retrieving revision 1.103
>diff -p -u -5 -r1.103 browser.css
>--- browser/themes/pinstripe/browser/browser.css	5 Jan 2008 06:29:08 -0000	1.103
>+++ browser/themes/pinstripe/browser/browser.css	16 Jan 2008 19:04:21 -0000
>@@ -1618,10 +1618,15 @@ toolbarbutton.bookmark-item[dragover="tr
>   margin: -1px 0 -2px;
>   padding: 1px 2px 2px 0;
>   border-right: 1px solid #888;
>   background-color: white;
>   opacity: 0.9;
>+  -moz-user-focus: normal;
>+}
>+
>+#identity-box:focus {
>+  outline: 1.4pt solid -moz-mac-focusring;

this needs to be tested carefully, the location bar itself has no focusring on mac.


>Index: toolkit/content/widgets/autocomplete.xml
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v
>retrieving revision 1.104
>diff -p -u -5 -r1.104 autocomplete.xml
>--- toolkit/content/widgets/autocomplete.xml	3 Jan 2008 05:52:37 -0000	1.104
>+++ toolkit/content/widgets/autocomplete.xml	16 Jan 2008 19:04:25 -0000
>@@ -384,10 +384,14 @@
>       <!-- ::::::::::::: key handling ::::::::::::: -->
> 
>       <method name="onKeyPress">
>         <parameter name="aEvent"/>
>         <body><![CDATA[
>+          if (aEvent.target.localName != "textbox") {
>+            return;  // Let child buttons of autocomplete take input
>+          }
>+            

nit: unnecessary braces and trailing spaces.
Attachment #297383 - Flags: review?(mano) → review-
>>+    
>>+    if (event.type == "keypress") {
>>+      // If opened with keyboard, focus the more info link
>>+      setTimeout(this.focusById, 0, 'identity-popup-more-info-link');
>>+    }

> See above. And I think you could use a popupshown handler, rather than
> setTimeout.

Mano, I only wanted to focus the link when the popup was opened with the keyboard. Does that make sense?

>>-          <box id="identity-box"
>>-               onclick="getIdentityHandler().handleIdentityClick(event);">>
>>+          <box id="identity-box" role="button"
>>+               onclick="getIdentityHandler().showIdentityPopup(event);"
>>+               onkeypress="getIdentityHandler().showIdentityPopup(event);"
>>+               tooltiptext="&proxyIcon.tooltip;">>
>>             <hbox align="center">>
>>               <deck id="page-proxy-deck" onclick="PageProxyClickHandler(event);">>
>>                 <image id="page-proxy-button"
>>-                       ondraggesture="PageProxyDragGesture(event);"
>>-                       tooltiptext="&proxyIcon.tooltip;"/>>
>>+                       ondraggesture="PageProxyDragGesture(event);"/>>
>>                 <image id="page-proxy-favicon" validate="never"
>>                        ondraggesture="PageProxyDragGesture(event);"
>>                        onload="this.parentNode.selectedIndex = 1;
>>                                event.stopPropagation();"
>>                        onerror="this.removeAttribute('src');
>>-                                this.parentNode.selectedIndex = 0;"
>>-                       tooltiptext="&proxyIcon.tooltip;"/>>
>>+                                this.parentNode.selectedIndex = 0;"/>>

> what's this all about?

Do you mean the fact that we move the tooltiptext up? It needs to be on the item that receives focus otherwise there is no accessible name for it. I don't see why it was put on both children, when it works just as well on the parent.

> It would be cleaner to make this a xul:button (probably with 
> -moz-appearance:none)

I tried hard to make that a button, but it never looked quite right. I agree it is cleaner (not to mention correct semantics) to have it be a <button> instead of a <box>, and correct semantics help a11y a lot. However can we make that a follow-up bug and assign it to whoever understands why this is a box in the first place?
(In reply to comment #52)
> >     <!-- Popup for site identity information -->
> >-    <panel id="identity-popup" position="after_start" hidden="true" noautofocus="true">
> >+    <panel id="identity-popup" position="after_start" hidden="true" noautofocus="true"
> >+           onpopuphiding="document.getElementById('urlbar').focus();">
> 
> I cannot recall whether there's a way to open Larry if the location bar is
> hidden, please ask Jhonatan or Gavin.

As far as I know, there isn't a way to open Larry if the location bar is hidden. But I'm not sure we want to be making that assumption here. Why do we need to focus the URL bar onpopuphiding, anyways?
1) Waiting on an answer from Gavin on whether Larry can be opened when URL is closed
2) Will look into <panel label> question
3) Ready for Surkov to look at mozilla/accessible changes
Attachment #297383 - Attachment is obsolete: true
Attachment #297422 - Flags: review?(surkov.alexander)
Attachment #297383 - Flags: review?(surkov.alexander)
Larry is currently only launched onclick, so he isn't available when the location bar is hidden, however that will likely change with bug 404642.
Johnathan, bug 404642 is currently fixed by this patch, by allowing Shift+tab to focus the site button. Please see my last comment in that bug -- I think it should just be marked as dependent on this one if people agree on this solution.
> As far as I know, there isn't a way to open Larry if the location bar is
> hidden. But I'm not sure we want to be making that assumption here. Why do we
> need to focus the URL bar onpopuphiding, anyways?

Because we're making the identity button focusable with this patch, so that a user can shift+Tab to it. So that means if you click on it and then close the identity popup, focus would be on that little button, which would be annoying for mouse users.

Comment on attachment 297422 [details] [diff] [review]
Address most of Mano's comments


>   <binding id="autocomplete-base-popup" extends="chrome://global/content/bindings/popup.xml#popup">
>-    <implementation implements="nsIAutoCompletePopup">
>+    <implementation implements="nsIAutoCompletePopup,nsIAccessibleProvider">

iirc, you shouldn't write nsIAccessibleProvider because #popup has it already.

>       <property name="accessibleType" readonly="true">
>         <getter>
>           <![CDATA[
>-            return Components.interfaces.nsIAccessibleProvider.XULImage;
>+            // Developers have a habit of using XUL images with an onclick
>+            // handler for what is semantically a toolbarbutton
>+            return this.hasAttribute("onclick") ?
>+              Components.interfaces.nsIAccessibleProvider.XULToolbarButton :
>+              Components.interfaces.nsIAccessibleProvider.XULImage;

I'm not sure we should handle this in this bug and not sure we should handle this at all. This is XUL. And if XUL developers needs more than usual button then I guess they should care about accessibility of their new widget. I would 100% agree for HTML but XUL. Where did you notice this habit of XUL developers?

>+  const long NoAccessible = 0; // Do not create an accessible for this object

I would like to have a bit wider documentation, would you say you should use this constant to make unaccessible the binding which is inherited from accessible binding. Something like this.

the rest looks ok, r=me.
Attachment #297422 - Flags: review?(surkov.alexander) → review+
(In reply to comment #58)
> Johnathan, bug 404642 is currently fixed by this patch, by allowing Shift+tab
> to focus the site button. Please see my last comment in that bug -- I think it
> should just be marked as dependent on this one if people agree on this
> solution.

This sounds fine to me, I've tagged it as a dependent. When this one is resolved, I'll mark that one fixed as well.

(In reply to comment #59)
> > As far as I know, there isn't a way to open Larry if the location bar is
> > hidden. But I'm not sure we want to be making that assumption here. Why do we
> > need to focus the URL bar onpopuphiding, anyways?
> 
> Because we're making the identity button focusable with this patch, so that a
> user can shift+Tab to it. So that means if you click on it and then close the
> identity popup, focus would be on that little button, which would be annoying
> for mouse users.

So yeah, since there is no longer danger of some *other* way to launch larry showing up in the near future, the line in question is probably safe, but it still seems like it wouldn't hurt terribly to check if urlbar exists before giving it focus, as future proofing?  Maybe that's overkill...

Johnathan, how about if I just wrap it in try/catch?
(In reply to comment #62)
> Johnathan, how about if I just wrap it in try/catch?

Hmm.  A try/catch risks masking other exceptions, but in this case I don't know how likely that is (a bug in our focus() implementation maybe?)  WFM, but it's Mano's review comment.  :)
(In reply to comment #53)
> >>+    
> >>+    if (event.type == "keypress") {
> >>+      // If opened with keyboard, focus the more info link
> >>+      setTimeout(this.focusById, 0, 'identity-popup-more-info-link');
> >>+    }
> 
> > See above. And I think you could use a popupshown handler, rather than
> > setTimeout.
> 
> Mano, I only wanted to focus the link when the popup was opened with the
> keyboard. Does that make sense?
> 

You could still use an event handler for that, something like:
 
     if (event.type == "keypress") {
       var popupShownHandler = function(popupEvent) {
         document.getElementById("identity-popup-more-info-link").focus();
         popupEvent.target.removeEventListener("popupshown", popupShownHandler, false);
       }
       this._identityPopup.addEventListener("popupshown", popupShownHandler, false);
 
     }

     // Now open the popup, anchored off the primary chrome element
     this._identityPopup.openPopup(this._identityBox, 'after_start');

Note the commands order change here.

That said, I couldn't figure out why may you only focus it on-keypress.

> > what's this all about?
> 
> Do you mean the fact that we move the tooltiptext up? It needs to be on the
> item that receives focus otherwise there is no accessible name for it. I don't
> see why it was put on both children, when it works just as well on the parent.
> 

ok
This breaks shift+tab from the location-bar to the content area. :( What sort of info does Larry expos which is not accessible through the Page Info dialog?
(In reply to comment #64)
> > > what's this all about?
> > 
> > Do you mean the fact that we move the tooltiptext up? It needs to be on the
> > item that receives focus otherwise there is no accessible name for it. I don't
> > see why it was put on both children, when it works just as well on the parent.
> > 
> 
> ok

Actually, that may create a problem, sorry I didn't catch it sooner.  Moving the tooltip up to the box puts it in contention with the code that sets the box-level tooltip here:

http://mxr.mozilla.org/mozilla/source/browser/base/content/browser.js#5684 

Leaving it on the icons allows the old feedback about dragging to persist while still giving identity verification information on the rest of the box's surface.  Since then, however, the design of the box has changed so that, in the non-SSL case, there is almost no remaining surface. This makes the current state of things sort of confusing.

I'm surprised this was necessary, since the tooltip should be set on the box by that code in all cases.  Were you being pro-active here, Aaron, or did the tooltip set in code not work properly?

We could just eliminate the tooltip attrs entirely, and let browser.js handle setting it.  This would eliminate the multiple-tooltips behaviour in current builds, which is probably a win of its own, but not if doing that breaks the accessibility of these changes.  In any case, moving the attribute up is not likely to have the intended effect (since I expect it will just get blown away).

(In reply to comment #65)
> This breaks shift+tab from the location-bar to the content area. :( What sort
> of info does Larry expos which is not accessible through the Page Info dialog?

None - Larry is a subset of page info.  But Marco makes the case in bug 404642 comment 2 that it should be keyboard accessible anyhow, and beltzner points out (bug 404642 comment 4) that the panel might be expanded post-FF3 to include other content, making it more important again that it be keyboard accessible (though there he suggests alt-down as the shortcut.)

> That said, I couldn't figure out why may you only focus it on-keypress.
Because in the case of opening it with the mouse, you just want focus to return to where it was.

(In reply to comment #65)
> This breaks shift+tab from the location-bar to the content area. :( What sort
> of info does Larry expos which is not accessible through the Page Info dialog?
It doesn't break it. It just means there is one more shift+tab required. This doesn't significantly clutter the tab order since it's behind the location bar.

> None - Larry is a subset of page info.  But Marco makes the case in bug 404642
> comment 2 that it should be keyboard accessible anyhow, and beltzner points out
> (bug 404642 comment 4) that the panel might be expanded post-FF3 to include
> other content, making it more important again that it be keyboard accessible
> (though there he suggests alt-down as the shortcut.)
Alt+down would be a bit odd since that always means open up a list of choices. Users wouldn't expect that and would be less likely to discover the feature.

(In reply to comment #67)
> > That said, I couldn't figure out why may you only focus it on-keypress.
> Because in the case of opening it with the mouse, you just want focus to return
> to where it was.

Sorry, I answered the wrong question here.

I don't mind focusing it when the popup is opened with the mouse, if that suits everyone. It simplifies the code.
> >       <property name="accessibleType" readonly="true">
> >         <getter>
> >           <![CDATA[
> >-            return Components.interfaces.nsIAccessibleProvider.XULImage;
> >+            // Developers have a habit of using XUL images with an onclick
> >+            // handler for what is semantically a toolbarbutton
> >+            return this.hasAttribute("onclick") ?
> >+              Components.interfaces.nsIAccessibleProvider.XULToolbarButton :
> >+              Components.interfaces.nsIAccessibleProvider.XULImage;
> 
> I'm not sure we should handle this in this bug and not sure we should handle
> this at all. This is XUL. And if XUL developers needs more than usual button
> then I guess they should care about accessibility of their new widget. I would
> 100% agree for HTML but XUL. Where did you notice this habit of XUL developers?

I agree the identity button should be a xul:button but I couldn't get that to render the same way. Maybe it is because of the children. I don't know. Maybe dao could help. I will file a follow-up bug if we don't get it this time.
Umm, the identity button is a box, not an image. So how's that related to the cited code?
Sorry, we have the same problem in both places. It is a problem if someone uses either an <image> or <box> when they mean a button, because the a11y code doesn't realize it's a button.
Comment on attachment 297422 [details] [diff] [review]
Address most of Mano's comments

>+    if (event.type == "keypress") {
>+      // If opened with keyboard, focus the more info link
>+      setTimeout(document.getElementById('identity-popup-more-info-link').focus(), 0);

setTimeout expects a function, so this is wrong.

>-          <box id="identity-box"
>-               onclick="getIdentityHandler().handleIdentityClick(event);">
>+          <box id="identity-box" role="button"
>+               onclick="getIdentityHandler().handleIdentityButtonEvent(event);"
>+               onkeypress="getIdentityHandler().handleIdentityButtonEvent(event);"
>+               tooltiptext="&proxyIcon.tooltip;">
>             <hbox align="center">
>               <deck id="page-proxy-deck" onclick="PageProxyClickHandler(event);">
>                 <image id="page-proxy-button"
>-                       ondraggesture="PageProxyDragGesture(event);"
>-                       tooltiptext="&proxyIcon.tooltip;"/>
>+                       ondraggesture="PageProxyDragGesture(event);"/>
>                 <image id="page-proxy-favicon" validate="never"
>                        ondraggesture="PageProxyDragGesture(event);"
>                        onload="this.parentNode.selectedIndex = 1;
>                                event.stopPropagation();"
>                        onerror="this.removeAttribute('src');
>-                                this.parentNode.selectedIndex = 0;"
>-                       tooltiptext="&proxyIcon.tooltip;"/>
>+                                this.parentNode.selectedIndex = 0;"/>
>               </deck>

&proxyIcon.tooltip; is "Drag and drop this icon to create a link to this page", which is wrong for #identity-box. See setIdentityMessages in browser.js for where we set a tooltiptext for #identity-box.
> Actually, that may create a problem, sorry I didn't catch it sooner.  Moving
> the tooltip up to the box puts it in contention with the code that sets the
> box-level tooltip here:
> 
> http://mxr.mozilla.org/mozilla/source/browser/base/content/browser.js#5684 

I think there is already contention between 2 things:
"This website does not supply identity information"
and
"Drag and drop this image to create a link to this page"

Which is supposed to show up when?

> We could just eliminate the tooltip attrs entirely, and let browser.js handle
> setting it.  This would eliminate the multiple-tooltips behaviour in current
> builds, which is probably a win of its own, but not if doing that breaks the
> accessibility of these changes. 
What is the multiple-tooltips behavior, I don't see it?
(In reply to comment #67)
> > None - Larry is a subset of page info.  But Marco makes the case in bug 404642
> > comment 2 that it should be keyboard accessible anyhow, and beltzner points out
> > (bug 404642 comment 4) that the panel might be expanded post-FF3 to include
> > other content, making it more important again that it be keyboard accessible
> > (though there he suggests alt-down as the shortcut.)
> Alt+down would be a bit odd since that always means open up a list of choices.
> Users wouldn't expect that and would be less likely to discover the feature.

I agree with Aaron here. Shift+Tabbing from the Location bar is intuitive,
Alt+Down is out of the question, IMO, since that always means "open a combobox"
or the like, and other keystrokes are also to be learned. Being in the tab
order means it is discoverable for keyboard users as well as sighted people
using a mouse.
Johnathan, it turns out that if I return the tooltiptext to where it was it turns out I get the ideal behavior. The tooltiptext is exactly as it is now. But, the accessible name is more appropriate. Instead of getting the drag and drop message, screen reader users hear that the website has no identity information.
(In reply to comment #71)
> Sorry, we have the same problem in both places. It is a problem if someone uses
> either an <image> or <box> when they mean a button, because the a11y code
> doesn't realize it's a button.

I can make the box a button.
(In reply to comment #75)
> Johnathan, it turns out that if I return the tooltiptext to where it was it
> turns out I get the ideal behavior. The tooltiptext is exactly as it is now.
> But, the accessible name is more appropriate. Instead of getting the drag and
> drop message, screen reader users hear that the website has no identity
> information.

Aaron - I had a long explanation of why I thought that was the way to go, but I midair collided with you beating me to it, so that sounds good to me.  :)
Ok, we can file a follow-up bug on that. My apologies for getting the 2 issues confused. xul:image is a separate problem that Surkov brought up.

Surkov, xul:image is used with onclick in several places in the browser. Yes, we can keep trying to remind developers to use the right XUL elements but it doesn't always help. Developers forget, and it is better to be safe than sorry. Anything with an onclick needs to be exposed as a widget. We already do this for xul:label -- if it has an onclick it becomes a link.
Comment on attachment 297422 [details] [diff] [review]
Address most of Mano's comments

>Index: browser/themes/winstripe/browser/browser.css

>   outline: 1px solid ThreeDShadow;
>   -moz-margin-start: -1px;
>   /* currently, the identity box is always LTR */
>   -moz-outline-radius-topleft: 2px;
>   -moz-outline-radius-bottomleft: 2px;
>+  -moz-user-focus: normal;
>+}
>+
>+#identity-box:focus {
>+  outline: 1px dotted -moz-DialogText;
> }

#identity-box already has an outline without focus :(
Yes but this makes the outline dotted. I don't have a better way to show focus -- it seems okay. Do you have a better idea?
There's an inner hbox on which you could add an outline... or wait for my button patch and use |.button-box|.
Attached patch Address comments (obsolete) — Splinter Review
When the identity popup goes away it really looks better to focus the address bar. It's kind of like hitting the down arrow to the right of the address bar and then escape. Since it's part of the address bar the address bar gets focus.

I still needed a setTimeout before focusing the address bar in the popuphidden event, otherwise the tab order is confused after focus is set. I'm not 100% sure why that is but I see all the places we focus the URL bar in browser.js also do the setTimeout().
Attachment #297586 - Flags: review?(mano)
So, as things stand, we focus the content area when the popup opened by the star button gets hidden. See
http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser-places.js#102

1. Does your patch break that? Note that there's a bug filed for returning the focus to where it was.
2. Should we focus the content area in this case too?
Mano, this fixes the bug to return the focus to where it was when a panel closes. That's bug 402499, which now depends on this one.

It's a lot harder to do for the identity popup since that button is now focusable. I like what we did for the identity popup for reasons described in comment 82.
Attachment #297586 - Attachment is obsolete: true
Attachment #297602 - Flags: review?(mano)
Attachment #297586 - Flags: review?(mano)
Comment on attachment 297602 [details] [diff] [review]
Remove extra focus code from browser-places.js

Enn should review the popups/panels  changes first.
Attachment #297602 - Flags: review?(mano)
1) Seeking UI review for shift+tab change discussed in this bug (shift+tab to focus identity popup). Closing that panel puts focus/selection on the URL bar (as already happens when opening & closing history with dropdownmarker for URL bar)

2) Seeking enn's review for norestorefocus attribute for <panel>
Attachment #297602 - Attachment is obsolete: true
Attachment #297615 - Flags: ui-review?(beltzner)
Attachment #297615 - Flags: review?(mano)
Attachment #297615 - Flags: review?(enndeakin)
Comment on attachment 297615 [details] [diff] [review]
1) Add support for norestorefocus="true" in <panel> and use it in bookmark panel, 2) use label attribute instead of hidden <label> for bookmark panel

r=mano for the browser/ changes, thanks.
Attachment #297615 - Flags: review?(mano) → review+
(In reply to comment #78)
> Ok, we can file a follow-up bug on that. My apologies for getting the 2 issues
> confused. xul:image is a separate problem that Surkov brought up.
> 
> Surkov, xul:image is used with onclick in several places in the browser. Yes,
> we can keep trying to remind developers to use the right XUL elements but it
> doesn't always help. Developers forget, and it is better to be safe than sorry.
> Anything with an onclick needs to be exposed as a widget. We already do this
> for xul:label -- if it has an onclick it becomes a link.
> 

Ok, if you want to expose xul:image with registered click event handler then should we expose anything with registered click event handler?

I'm quite not sure our toolbarbutton or button accessibles are suitable for this because these accessibles have specific code for buttons. Probably we should have separate class for this stuff. Btw, why did you use toolbarbutton accessible instead of button accessible?
(In reply to comment #89)

> > Anything with an onclick needs to be exposed as a widget. We already do this
> > for xul:label -- if it has an onclick it becomes a link.
> > 

That's a bug though (353345), and should never have been implemented that way.

> 
> Ok, if you want to expose xul:image with registered click event handler then
> should we expose anything with registered click event handler?
> 

Actually, I don't know enough about the accessibility api works to know what should be done. At first I thought that the image change was ok, but I'm not sure any more. An <image> is still an image even with a click handler. It doesn't magically become a button. If this change isn't done, is there no means for accessible tools to emulate clicks on the image?
The xul:image button is spun off to bug 412849.
Attachment #297615 - Flags: review?(enndeakin) → review+
Whiteboard: Status: need ui+=beltzner, get in ASAP so it has adequate testing
Comment on attachment 297615 [details] [diff] [review]
1) Add support for norestorefocus="true" in <panel> and use it in bookmark panel, 2) use label attribute instead of hidden <label> for bookmark panel

AIUI, this puts Larry in the tab order, and that's the only user-facing change here.  That seems eminently reasonable, thus ui-r=me
Attachment #297615 - Flags: ui-review?(beltzner) → ui-review+
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: Status: need ui+=beltzner, get in ASAP so it has adequate testing
Unfortunately I had to back out because of the orange:
*** 645 ERROR FAIL | noautofocus command event fired | got false, expected true | chrome://mochikit/content/chrome/toolkit/content/tests/chrome/test_panel_focus.xul
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
In the test one panel opens the other in its onpopuphidden handler. However, the popup's new focus restore feature is change the focus when the first popup closes, making the test think that the second popup changed the focus. The easiest thing for now is to just not have the first popup restore focus.
Attachment #298645 - Flags: review?(enndeakin)
Comment on attachment 298645 [details] [diff] [review]
Correct the test so that it doesn't fail

Please file a follow up on testing both behaviors.
Attachment #298645 - Flags: review?(enndeakin) → review+
Spun off bug 413750.
Blocks: 413750
Aaron, can this not be marked RESOLVED FIXED after you checked in the fix for the failing test?
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Depends on: 415791
Verified using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Status: RESOLVED → VERIFIED
Depends on: 417257
Blocks: 428915
Depends on: 449040
Depends on: 428602
No longer depends on: 445727
Depends on: 495672
Depends on: 758288
You need to log in before you can comment on or make changes to this bug.