673958 - rework accessible focus handling

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Root accessible listen focus and blur and few other DOM events to manage the accessible focus, also provides few methods to fire focus event. There's nothing specific to root accessible to manage the focus.

We have couple global variables to keep focus related information. Including the last focused node. In most cases we don't need to store last focused node and should rely on DOM focus manager instead (bug 495624), however we should store focused accessible in case of menus, listboxes and few other controls.

AccEvent has some hacks to calculate target for XUL trees (used for focus event too), see bug 386821. For that we should make accessible events operate on accessible tree rather than on DOM tree (neither constructors taking DOM node nor coalescing by DOM nodes).

Comment 1

[alexander :surkov (:asurkov)]

Enn, does make sense to hook into focus manager to make him call accessibility methods directly rather than deal with DOM focus/blur events? Would it make the code simpler/faster?

Comment 2

[Neil Deakin]

Yes, I think it should be calling accessibility methods directly rather than using events. Same with other areas such as menus.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> Yes, I think it should be calling accessibility methods directly rather than
> using events.

could you suggest me where I should add hooks please?

> Same with other areas such as menus.

Exactly, they fire accessible-only DOM events which is ugly hack.

Comment 4

[Neil Deakin]

Can you make a list of which notifications you need?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> Can you make a list of which notifications you need?

when control or document gets focus (exclusive, i.e. if control get focus then we don't need document focus) and when control loose focus but document stays focused.

For other stuffs like radios, menus, listboxes and trees we watch for DOM events like popuphiding, DOMMenuItemActive, DOMMenuBarInactive and DOMMenuBarInactive. But it sounds I can just replace these events on a11y calls.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> events like popuphiding, DOMMenuItemActive, DOMMenuBarInactive and
> DOMMenuBarInactive. But it sounds I can just replace these events on a11y
> calls.

of course except popuphiding

Comment 7

[alexander :surkov (:asurkov)]

Enn, ping please?

Comment 8

[Neil Deakin]

Well that's not really specific enough, but you may want to look at where the events are fired (calls to nsFocusManager::SendFocusOrBlurEvent) and determine what you actually want.

It may make sense in the future to expand upon the concept of focus by providing a second concept of 'this is the thing the user is examining or manipulating' which encompasses not only the keyboard focus but mouse usage through menus, context menus, etc.

Comment 9

[alexander :surkov (:asurkov)]

Enn, should DOM document be considered focusable always in means focus event can be fired for any DOM document (HTML or XUL)?

Comment 10

[Neil Deakin]

A focus event can be fired on any document and window.

Comment 11

[alexander :surkov (:asurkov)]

Neil, in the case of editable iframe@src="about:blank" (i.e. iframe.contentDocument.designMode="on") the DOM focus event is fired for DOM document and window but nsFocusManager::GetFocusedContent() returns HTML element (document root element) and there's no DOM focus event for the HTML element. Is that expected behavior or a bug?

Comment 12

[Neil Deakin]

Design mode documents don't have focused root content. GetFocusedContent should probably be returning null here.

You could also just check doc->HasFlag(NODE_IS_EDITABLE) for this situation.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: wip

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 557873 [details] [diff] [review]
wip

There's some nice cleanup here.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: wip2

Comment 16

[alexander :surkov (:asurkov)]

Attachment: wip3

Comment 17

[alexander :surkov (:asurkov)]

Attachment: wip4

Comment 18

[alexander :surkov (:asurkov)]

Attachment: wip5

encapsulate focus processing logic into accessible classes by new Widgets interface

Comment 19

[alexander :surkov (:asurkov)]

try server build http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d7eceb009fca/

Comment 20

[alexander :surkov (:asurkov)]

Attachment: wip6

Comment 21

[Marco Zehe (:MarcoZ)]

The only regression I found so far is this:
1. Press Ctrl+K to focus the Search box.
2. Press F4 to drop down the list of available search engines.
3. Start arrowing up and down to select a search engine.

Expected: NVDA would speak the currently focused item. It does so in current nightly.
Actual: "Blank" is spoken, and there's no indication that something is actually happening.

Comment 22

[Marco Zehe (:MarcoZ)]

On positive notes:
1. Back and Forward now give nice speech feedback with NVDA, indicating that a page is reloading, putting the focus right where it left off, and NVDA starts reading downwards from there.
2. Getting into Textareas like when editing a Wiki page seems to work more smoothly, with all the features being available right away. Caret navigation working etc.
3. Menus are working nicely. Comboboxes are, too.

Jamie, perhaps you can verify some of the bugs you filed and which Surkov marked as being blocked by this one to verify your API calls give you what you expect to see now.

I will also go through the bugs in more detail when I get back to Germany. But testing shows that focus handling seems to work a lot better with this patch! Great work!

Comment 23

[alexander :surkov (:asurkov)]

Attachment: wip7

build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-acc172f85d85/

Comment 24

[alexander :surkov (:asurkov)]

Attachment: patch

Boris, please review dom and layout parts.

Marco and David, if you get a chance to run through code (doesn't matter how deep) then please let me know your findings.

Comment 25

[alexander :surkov (:asurkov)]

Attachment: patch2

polish Widgets interface

Comment 26

[alexander :surkov (:asurkov)]

latest try server build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e0c405c742e4 (contains a small fix for random focus menu problem)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 560856 [details] [diff] [review]
patch2

So which part of that patch do you actually want me to review?

Please rerequest review with that information.

If the answer is "the whole thing", then "r-, patch too large".

Comment 28

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky (:bz) from comment #27)
> Comment on attachment 560856 [details] [diff] [review]
> patch2
> 
> So which part of that patch do you actually want me to review?
> 
> Please rerequest review with that information.
> 
> If the answer is "the whole thing", then "r-, patch too large".

Boris, I asked you to review dom and layout parts in comment #24. 
dom/base (nsFocusManager.cpp)
layout/xul (nsIListControlFrame.h, nsListControlFrame.cpp, nsListControlFrame.h, nsMenuBarFrame.cpp, nsMenuBarFrame.h, nsMenuPopupFrame.cpp, nsMenuPopupFrame.h and nsTreeSelection.cpp)

These files are all placed in the end of patch.

Comment 29

[alexander :surkov (:asurkov)]

Attachment: dom/base part

Comment 30

[alexander :surkov (:asurkov)]

Attachment: layout part

Comment 31

[alexander :surkov (:asurkov)]

Attachment: accessible part

Comment 32

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Sorry I missed that; the Bugzilla change noise drowned it out.

Is the change to nsTreeSelection.cpp compatible (in the sense that no one expects the old event when aIndex == -1)?

nsMenuBarFrame.h declares the wrong query frame target.

Someone who understands focus and events (e.g. smaug or enn) should review the focus manager changes, and Mats should review the list control frame changes.

Comment 33

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky (:bz) from comment #32)
> Ah, ok.  Sorry I missed that; the Bugzilla change noise drowned it out.
> 
> Is the change to nsTreeSelection.cpp compatible (in the sense that no one
> expects the old event when aIndex == -1)?

Originally DOMMenuItemActive/Inacitve events were introduced for a11y but nowdays they are used in some XBL bindings and even have some automated tests. However I didn't see that they are used for XUL trees outside of a11y.
 
> nsMenuBarFrame.h declares the wrong query frame target.

will fix

> Someone who understands focus and events (e.g. smaug or enn) should review
> the focus manager changes, and Mats should review the list control frame
> changes.

thank you

Comment 34

[Marco Zehe (:MarcoZ)]

The latest try-server build fixes the problems I was seeing in the Search bar Search Engine selection list.

There was one instance where I didn't get the NVDA virtual document to work at all any more, but couldn't reproduce later, and I am not sure how I got into this state.

Continuing testing.

Comment 35

[Marco Zehe (:MarcoZ)]

When performing a search on Google, virtual document for NVDA is never reinstantiated:
1. Go to www.google.com.
2. Type in anything. If you have braille, notice that we still have the problem that the text interface is not correctly returning caret offsets still. Braille won't show what one is typing.
3. Tab to the "Search" field.

In a regular nightly, virtual mode with NVDA gets reenabled, and one can navigate the document using arrow keys.
In the try build, virtual mode is not reenabled, focus gets lost, one cannot really do anything with this document at this stage any more.

Comment 36

[Marco Zehe (:MarcoZ)]

Comment on attachment 560962 [details] [diff] [review]
accessible part

denying review based on last comment. This is a pretty serious regression breaking one of the base functionality in the browser (Google searches).

Comment 37

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #35)
> When performing a search on Google, virtual document for NVDA is never
> reinstantiated:
> 1. Go to www.google.com.
> 2. Type in anything. If you have braille, notice that we still have the
> problem that the text interface is not correctly returning caret offsets
> still. Braille won't show what one is typing.
> 3. Tab to the "Search" field.
> 
> In a regular nightly, virtual mode with NVDA gets reenabled, and one can
> navigate the document using arrow keys.
> In the try build, virtual mode is not reenabled, focus gets lost, one cannot
> really do anything with this document at this stage any more.

Behavior depends on string you typed and available search results. When you type symbols, then autocomplete popup appears. If your search string can't be autocompleted (for example, 'hello') then focus goes to search button when you tab. Otherwise (for example 'he') the focus stays on input and value is changed to 'hello' (first search suggestion). Second tab moves you to search button. Is that what you observe?

Comment 38

[Marco Zehe (:MarcoZ)]

No. I used the same search term, and I noticed the autocomplete, but in a regular build, when continuing to tab, and landing on the Search button, virtual buffer would be turned on eventually. In the try build, it won't.

Comment 39

[alexander :surkov (:asurkov)]

Can you give me complete steps to reproduce then? I don't see a difference between regular build and try build.

Comment 40

[Marco Zehe (:MarcoZ)]

1. Go to google.com.
2. Focus mode is turned on automatically. Type in something like "test" (without the quotes).
3. Tab. You may land on the autocomplete thing, but nothing gets spoken.
4. Tab again. In regular nightly, I now get virtual buffer turned back on, indicated by the beeping sound NVDA makes. In try build, I don't hear this. Instead, silence.

Comment 41

[alexander :surkov (:asurkov)]

I definitely hear high "poom" sound when I tab out the search input and low "chick" when I go back.

Comment 42

[alexander :surkov (:asurkov)]

What does turning on/off vb mean from API point of view? 

Can you see this problem on any other try server builds?

Comment 43

[Marco Zehe (:MarcoZ)]

This is the first try that I saw this on. From API point of view, it means that it is not recognized that focus moved back to the document and onto an element that does not require focus mode.

Comment 44

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #43)
> This is the first try that I saw this on.

I'll file new try build against trunk just in case. Builds aren't different too much to have differences like this one.

> From API point of view, it means
> that it is not recognized that focus moved back to the document and onto an
> element that does not require focus mode.

Focus shouldn't leave document in this case. Can you watch focus events and list them here?

Comment 45

[Marco Zehe (:MarcoZ)]

Comment on attachment 560962 [details] [diff] [review]
accessible part

reviewing the tests only for now:

>diff --git a/accessible/tests/mochitest/attributes/test_obj_group.xul 
...
>+    gA11yEventDumpToConsole = true; // debug stuff

Don't forget to comment this before landing.

In autocomplete.js, should we not document all params to all functions? I notice that some you do document, others you don't, with others yet again, you only document part of the parameters.

>+ * Invokers defined below take a checker object (or array of checker objects).
>+ * An invoker listen default event type registered in event queue object until

"an invoker listens for ..."

>+ * it checker is provided.

"its checker..."

(Note left off at events/makefile.in)

Comment 46

[alexander :surkov (:asurkov)]

latest against trunk: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cf52ef53044f/

Comment 47

[Marco Zehe (:MarcoZ)]

Comment on attachment 560962 [details] [diff] [review]
accessible part

f=me for the patch based on the latest try-server build. The fluke I was seeing on the Google home page does no longer happen with this try build. Now on to further reviews...

Comment 48

[Marco Zehe (:MarcoZ)]

Comment on attachment 560962 [details] [diff] [review]
accessible part

>+  <div id="menubar2" role="menubar">
>+    <div id="menu-help" role="menuitem" tabindex="0">
>+      Help
>+      <div id="menupopup-help" role="menu" style="display: none;">

Huh, and this works? Because display: none; causes the accessible, and AFAIK all its children, to not be rendered into the a11y tree.

>+      // open sarch engline popup, no focus

Nit: fix spelling of "search" and "engine". ;)

>+      // no focus events for checkbox or radio inputs when they are checked
>+      // programmatically
>+      gQueue.push(new changeCurrentItem("checkbox"));
>+      gQueue.push(new changeCurrentItem("radio1"));

Does it make sense to test whether we fire statechange events here? I know these are focus tests, but...

>+/*
>+# activedesc, when focused add children and change attr
>+*/

What's that comment doing there? It is right before SimpleTest.waitForExplicitFinish();. Looks like you forgot to remove something.

>+      if (WIN) {
>+        // Alt key is used to active menubar and focus menu item on Windows,
>+        // other platforms requires setting a ui.key.menuAccessKeyFocuses
>+        // preference.
>+        gQueue.push(new toggleTopMenu(editableDoc, new topMenuChecker()));
>+        gQueue.push(new toggleTopMenu(editableDoc, new focusChecker(editableDoc)));
>+      }

Hint: On Linux, you can always press F10 to bring up the top menu bar. On Mac, it's CTRL+F2 (or CTRL+FN+F2 on notebooks). These are universally available keys (F10 also works on Windows similar to the alt key). Perhaps you can broaden this test to include at least Linux? The same goes for test_focus_menu.xul.

>+        title="Expanded state change events tests for comboboboxes and autocompletes.">

Nit: One "bo" too much in "comboboxes".

>+      todo(false, "Autocompletes don't fire expanded state change events when popup open!");

Do we have a bug on file for this? If so, please add a reference to it somehow.

r=me for the test parts with the above fixed/answered. Very, very nicely done test suite!

Comment 49

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #48)
> Comment on attachment 560962 [details] [diff] [review]
> accessible part
> 
> >+  <div id="menubar2" role="menubar">
> >+    <div id="menu-help" role="menuitem" tabindex="0">
> >+      Help
> >+      <div id="menupopup-help" role="menu" style="display: none;">
> 
> Huh, and this works? Because display: none; causes the accessible, and AFAIK
> all its children, to not be rendered into the a11y tree.

see invokers part, menu gets shown and events are fired. That's how ARIA is powerful ;)

> >+      // no focus events for checkbox or radio inputs when they are checked
> >+      // programmatically
> >+      gQueue.push(new changeCurrentItem("checkbox"));
> >+      gQueue.push(new changeCurrentItem("radio1"));
> 
> Does it make sense to test whether we fire statechange events here? I know
> these are focus tests, but...

that should be different tests, really. Otherwise it's really hard to say whether we test this case or not.

> >+/*
> >+# activedesc, when focused add children and change attr
> >+*/
> 
> What's that comment doing there? It is right before
> SimpleTest.waitForExplicitFinish();. Looks like you forgot to remove
> something.

that was a list of tests I should add. The last one was about aria-acitvedescendant, I just forgot to remove this when I added tests.

> 
> >+      if (WIN) {
> >+        // Alt key is used to active menubar and focus menu item on Windows,
> >+        // other platforms requires setting a ui.key.menuAccessKeyFocuses
> >+        // preference.
> >+        gQueue.push(new toggleTopMenu(editableDoc, new topMenuChecker()));
> >+        gQueue.push(new toggleTopMenu(editableDoc, new focusChecker(editableDoc)));
> >+      }
> 
> Hint: On Linux, you can always press F10 to bring up the top menu bar. On
> Mac, it's CTRL+F2 (or CTRL+FN+F2 on notebooks). These are universally
> available keys (F10 also works on Windows similar to the alt key). Perhaps
> you can broaden this test to include at least Linux? The same goes for
> test_focus_menu.xul.

I'll try that.

> >+      todo(false, "Autocompletes don't fire expanded state change events when popup open!");
> Do we have a bug on file for this? If so, please add a reference to it
> somehow.

iirc, we don't have. I'll file it.

> r=me for the test parts with the above fixed/answered. Very, very nicely
> done test suite!

thank you much!

Comment 50

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #48)

> Hint: On Linux, you can always press F10 to bring up the top menu bar. On
> Mac, it's CTRL+F2 (or CTRL+FN+F2 on notebooks). These are universally
> available keys (F10 also works on Windows similar to the alt key). Perhaps
> you can broaden this test to include at least Linux? The same goes for
> test_focus_menu.xul.

I didn't see F10 working on Linux due to some reason. Ctrl+F2 on mac activates top level menu while invoker may be used for in-page XUL menus. What I observe is F10 and alt works on Windows (through F10 doesn't show Firefox menu if it's hidden). So I think I'd skip this part since fixing it should take some time before I figure out how to test that on other platforms.

Comment 51

[Marco Zehe (:MarcoZ)]

(In reply to alexander surkov from comment #50)
> (In reply to Marco Zehe (:MarcoZ) from comment #48)
> 
> > Hint: On Linux, you can always press F10 to bring up the top menu bar. On
> > Mac, it's CTRL+F2 (or CTRL+FN+F2 on notebooks). These are universally
> > available keys (F10 also works on Windows similar to the alt key). Perhaps
> > you can broaden this test to include at least Linux? The same goes for
> > test_focus_menu.xul.
> I didn't see F10 working on Linux due to some reason. Ctrl+F2 on mac
> activates top level menu while invoker may be used for in-page XUL menus.
> What I observe is F10 and alt works on Windows (through F10 doesn't show
> Firefox menu if it's hidden). So I think I'd skip this part since fixing it
> should take some time before I figure out how to test that on other
> platforms.

Fine with me, thanks for looking into it!

Comment 52

[Marco Zehe (:MarcoZ)]

Tsts show that on Linux, comboboxes seem to be broken. The changing of items no longer is spoken. Affected comboboxes are:
Edit/Preferences/General page/When Firefox starts... combobox. In both dropped-down and non-dropped down states, changing the selection using up and down arrow no longer produces any speech with orca.

2. Select size="1" HTML elements in both dropped-down and non-dropped-down states, changing the selection does not produce any speech.

The Search field engine selection, however, works fine.

Comment 53

[alexander :surkov (:asurkov)]

I filed bug against Orca (https://bugzilla.gnome.org/show_bug.cgi?id=659839). Joanie said combobxes support is somehow broken (for example, expanded comboboxes aren't announced) and needs to be reworked in general. Hopefully that will be done in nearest future.

Comment 54

[Marco Zehe (:MarcoZ)]

Thanks Alex! OK, let's continue with reviews, let's see if we get Mats' review soon and ours done so we can possibly land this for Firefox 9 still. This has a lot of good fixes.

Comment 55

[alexander :surkov (:asurkov)]

Attachment: accessible part (v2)

fixed Marco's comments and fixed a menu issue

Comment 56

[Neil Deakin]

Comment on attachment 560961 [details] [diff] [review]
layout part

>diff --git a/layout/xul/base/src/nsMenuBarFrame.h b/layout/xul/base/src/nsMenuBarFrame.h
>--- a/layout/xul/base/src/nsMenuBarFrame.h
>+++ b/layout/xul/base/src/nsMenuBarFrame.h
>@@ -55,16 +55,18 @@
> 
> class nsIContent;
> 
> nsIFrame* NS_NewMenuBarFrame(nsIPresShell* aPresShell, nsStyleContext* aContext);
> 
> class nsMenuBarFrame : public nsBoxFrame, public nsMenuParent
> {
> public:
>+  NS_DECL_QUERYFRAME_TARGET(nsMenuPopupFrame)

You want to use 'nsMenuBarFrame' here

Comment 57

[Marco Zehe (:MarcoZ)]

Comment on attachment 561771 [details] [diff] [review]
accessible part (v2)

I gave the code part of the patch a read-over, and nothing stuck out at me.

Comment 58

[Trevor Saunders (:tbsaunde)]

Comment on attachment 561771 [details] [diff] [review]
accessible part (v2)

>+FocusManager::FocusedAccessible()

const?

>+FocusManager::IsFocusWithin(nsAccessible* aContainer)

const arg and make the method const?

>+FocusManager::IsInOrContainsFocus(nsAccessible* aAccessible)

same

>+FocusManager::DispatchFocusEvent(nsDocAccessible* aDocument,

const?

>+  inline bool IsActiveItem(nsAccessible* aAccessible)

const method and arg?

>+  inline bool HasDOMFocus(nsINode* aNode) const

const arg?

>+  nsIContent* FocusedDOMElm() const;

inline it?

>+private:

any reason for it? its reduntant no?

>+#define A11YDEBUG_FOCUS

make sure its undefined when you land it :)

>+#define A11YDEBUG_FOCUS_LOG_TIME                                               \
>+  {                                                                            \
>+    PRIntervalTime time = PR_IntervalNow();                                    \
>+    PRUint32 mins = (PR_IntervalToSeconds(time) / 60) % 60;                    \
>+    PRUint32 secs = PR_IntervalToSeconds(time) % 60;                           \
>+    PRUint32 msecs = PR_IntervalToMilliseconds(time) % 1000;                   \
>+    printf("Time: %2d:%2d.%3d\n", mins, secs, msecs);                          \
>+  }

I don't believe defining a block like that is portable, I think the write way to do it is do { blah; blah } while(0) but for ifdefed debug code I don't really care, I think it also works on gcc and I'll assume from you using it  works on msvc.

btw there is some more c++y timer stuff in mozilla/timer or something very similar, but again its debug code whatever works

>+    } break; // case eCoalesceOfSameType

I find the break on the same line as the brace a little odd, but in this case I think the braces aren't actually needed.

>+FocusManager* FocusMgr();

inline it?

>-  /**
>-   * Reference for accessibility service.
>-   */

I don't really care, but why the change?

>+bool
>+nsAccessible::IsWidget() const

why not use a bit in mFlags for this?

>+  bool IsLinkSelected();

why not keep it inline and export FocusManager.h?

> nsApplicationAccessible::FocusedChild()

I think we might be able to get rid of this impl since I think the new nsAccessible one works here no?

>   if (htmlInput) {
>     state |= states::SINGLE_LINE;
>   }
>   else {
>     state |= states::MULTI_LINE;
>   }

it'd be excellent if you changed that to state |= htmlInput ? states::SINGLELINE : states::MULTILINE; :)

>+  if (widget && widget-IsAutoComplete()) {
>+    state |= states::HASPOPUP | states::SUPPORTS_AUTOCOMPLETION;
>+    return state;

blank line before return?

>+nsHTMLTextFieldAccessible::ContainerWidget() const
>+{
>+  if (mParent && mParent->Role() == nsIAccessibleRole::ROLE_AUTOCOMPLETE)
>+    return mParent;
>+  return nsnull;

return (mparent && mParent->Role() == blah) ? mParent : nsnull; ?

>+    if (grandParent && grandParent->IsCombobox())
>+      return grandParent;
>+    return mParent;

blank line?

>diff --git a/accessible/src/xul/nsXULAlertAccessible.h b/accessible/src/xul/nsXULAlertAccessible.h
>-#endif  
>+#endif

what's this change about?

>+nsXULMenupopupAccessible::ContainerWidget() const

>+  // shouldn't be a real case

assert then?

btw its seems like we might want to think about killing the FocusedCHild() vfunc all together at some point.

Comment 59

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #58)

> >+  nsIContent* FocusedDOMElm() const;
> 
> inline it?

that requires to include "nsFocusManager.h", is it ok?

> >+private:
> 
> any reason for it? its reduntant no?

just style issue, usually we put one for member functions and for member variables.

> >+#define A11YDEBUG_FOCUS
> 
> make sure its undefined when you land it :)

sure

> I don't believe defining a block like that is portable, I think the write
> way to do it is do { blah; blah } while(0) but for ifdefed debug code I
> don't really care, I think it also works on gcc and I'll assume from you
> using it  works on msvc.
> 
> btw there is some more c++y timer stuff in mozilla/timer or something very
> similar, but again its debug code whatever works

yep, just debugging code, one day we should make it nicer

> >+    } break; // case eCoalesceOfSameType
> 
> I find the break on the same line as the brace a little odd, but in this
> case I think the braces aren't actually needed.

yes, just a style we use in this file

> >+FocusManager* FocusMgr();
> 
> inline it?

iirc, it should be prototyped before it's referred as a friend

> >-  /**
> >-   * Reference for accessibility service.
> >-   */
> 
> I don't really care, but why the change?

dunno, make restitution of it

> >+bool
> >+nsAccessible::IsWidget() const
> 
> why not use a bit in mFlags for this?

Good question because flags approach fits well now. I considered it but decided to stay on current approach since I foresee that widget status may depend on attributes presence (sure, we may want to recreate the accessible in this case).

I think I'd collected more data about Widget interface usage and then we can decide if it's ok to proceed with flags approach.

> >+  bool IsLinkSelected();
> 
> why not keep it inline and export FocusManager.h?

I think because we get includes collision, i.e when one file requires to include second one, and that second one requires the first one.

> > nsApplicationAccessible::FocusedChild()
> 
> I think we might be able to get rid of this impl since I think the new
> nsAccessible one works here no?

technically yes, but nsAccessible implementation is more general and that makes us to assume that application accessible can have a focus, that may be lead to misunderstandings.

> 
> >   if (htmlInput) {
> >     state |= states::SINGLE_LINE;
> >   }
> >   else {
> >     state |= states::MULTI_LINE;
> >   }
> 
> it'd be excellent if you changed that to state |= htmlInput ?
> states::SINGLELINE : states::MULTILINE; :)

fine with me

> >+  if (widget && widget-IsAutoComplete()) {
> >+    state |= states::HASPOPUP | states::SUPPORTS_AUTOCOMPLETION;
> >+    return state;
> 
> blank line before return?

not necessary I'd say

> >+nsHTMLTextFieldAccessible::ContainerWidget() const
> >+{
> >+  if (mParent && mParent->Role() == nsIAccessibleRole::ROLE_AUTOCOMPLETE)
> >+    return mParent;
> >+  return nsnull;
> 
> return (mparent && mParent->Role() == blah) ? mParent : nsnull; ?

ok

> >+    if (grandParent && grandParent->IsCombobox())
> >+      return grandParent;
> >+    return mParent;
> 
> blank line?

ok

> >diff --git a/accessible/src/xul/nsXULAlertAccessible.h b/accessible/src/xul/nsXULAlertAccessible.h
> >-#endif  
> >+#endif
> 
> what's this change about?

whitespaces in the end

> >+nsXULMenupopupAccessible::ContainerWidget() const
> 
> >+  // shouldn't be a real case
> 
> assert then?

fine with me

> btw its seems like we might want to think about killing the FocusedCHild()
> vfunc all together at some point.

yep, maybe we could do that one day

Comment 60

[alexander :surkov (:asurkov)]

Attachment: layout part (v2)

fixed queryframe_target issue for nsMenuPopupFrame

Comment 61

[alexander :surkov (:asurkov)]

Mats, can you share your review plans please? This is crucial bug for accessibility and I'd love to get it in asap.

Comment 62

[Mats Palmgren (inactive)]

I've started but not finished.  It looks mostly good, the one thing I'm not sure
about yet is the change in nsListControlFrame::PaintFocus.  It might help if
you could elaborate a bit on your intentions there.  Meanwhile, it's 6am here
and I need to get some sleep.  I'll try to finish the review before the Aurora
cut at 9am PDT (assuming that's what you meant by "asap").  No promises though.

Comment 63

[Mats Palmgren (inactive)]

What is the intended behavior when "2" is the current node?

<select multiple><option>0<option>1<option style="display:none">2<option>3

Comment 64

[alexander :surkov (:asurkov)]

(In reply to Mats Palmgren [:mats] from comment #62)
> I've started but not finished.  It looks mostly good, the one thing I'm not
> sure
> about yet is the change in nsListControlFrame::PaintFocus.  It might help if
> you could elaborate a bit on your intentions there.

I didn't wanted to change logic of PaintFocus but I see I changed it. So if all options are disabled then no focus is drawn. The patch makes it draw on first element. I can fix that I guess:

if (!focusedContent) {
  return;
}

childframe = focusedContent->GetPrimaryFrame();
if (!childFrame)
  etc


(In reply to Mats Palmgren [:mats] from comment #63)
> What is the intended behavior when "2" is the current node?
> 
> <select multiple><option>0<option>1<option style="display:none">2<option>3

I'd say that should fall into the case when no option is current, i.e. focus should be drawn on the first item.

Comment 65

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 66

[Mats Palmgren (inactive)]

Comment on attachment 562388 [details] [diff] [review]
layout part (v2)

>layout/forms/nsIListControlFrame.h

>   /**
>+   * Return current option. When listbox is focused then current option is
>+   * a focused option.
>+   */
>+  virtual already_AddRefed<nsIContent> GetCurrentOption() = 0;

I find the second sentence slightly misleading since an option never
has focus in the DOM sense.  I would prefer something like:
"The current option is the option displaying the focus ring when the
listbox is focused."

From an API point of view, I think it would be better if a11y avoided
direct use of frame classes as much as possible.
We could add a GetCurrentOption() to nsIDOMHTMLSelectElement or something
in the future which you could use instead.

>layout/forms/nsListControlFrame.cpp
>+  // If we have a current option then it's focused and get the child frame.

This comment is confusing; the code is obvious so I don't think we need
a comment for it:

>   nsIFrame * childframe = nsnull;
>-  nsresult result = NS_ERROR_FAILURE;
>+  nsCOMPtr<nsIContent> focusedContent = GetCurrentOption();
>+  if (focusedContent) {
>+    childframe = focusedContent->GetPrimaryFrame();
>+  }

(In reply to alexander surkov from comment #64)
> I didn't wanted to change logic of PaintFocus but I see I changed it. So if
> all options are disabled then no focus is drawn. The patch makes it draw on
> first element. I can fix that I guess:
> 
> if (!focusedContent) {
>   return;
> }
> 
> childframe = focusedContent->GetPrimaryFrame();
> if (!childFrame)
>   etc

That doesn't seem right, we do want to paint something I think.

I think you can restore almost the same behavior as we currently have
if you simply remove this block:

  if (!childframe) {
    // Try the first thing we have, but only if it's an element. Text frames
    // need not apply.
    childframe = containerFrame->GetFirstPrincipalChild();
    if (childframe && !childframe->GetContent()->IsElement()) {
      childframe = nsnull;
    }
  }

then we'll take the else-part with "CalcFallbackRowHeight()" instead.

I don't think what we are currently doing is right in some cases anyway
and I doubt we will break anything with this change since it only affects
the focus ring when the current option has display:none (rare edge case).

So either keep the PaintFocus() logic as you wrote it, or with removing
the block I suggested above is fine with me.

r=mats

Comment 67

[alexander :surkov (:asurkov)]

(In reply to Mats Palmgren [:mats] from comment #66)

> From an API point of view, I think it would be better if a11y avoided
> direct use of frame classes as much as possible.
> We could add a GetCurrentOption() to nsIDOMHTMLSelectElement or something
> in the future which you could use instead.

Accessibility code uses frame classes a lot, that shouldn't be not too bad, otherwise we'd need to dupe many layout things in content. But I like the idea to add currentOption property to nsIDOMHTMLSelectElement, it might be useful for scripting. Should I file bug for this?

Comment 68

[alexander :surkov (:asurkov)]

I started try server build with Mats comments fixed against trunk

Comment 69

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 561771 [details] [diff] [review]
accessible part (v2)

f=me great work.

Comment 70

[Marco Zehe (:MarcoZ)]

Marking this one blocking our e10s a11y meta bug since fixing this will go a long way towards making sure we'll also behave properly when e10s is turned on.

Comment 71

[alexander :surkov (:asurkov)]

landed on inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/30c186f6b48a

Comment 72

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/30c186f6b48a

Comment 73

[Marco Zehe (:MarcoZ)]

Verified that this landed and fixes a ton of other bugs (see the blocking list above), in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20110929 Firefox/10.0a1