rework accessible focus handling

VERIFIED FIXED in mozilla10

Status

()

defect
--
major
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Depends on 1 bug, Blocks 2 bugs, {access})

Trunk
mozilla10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(4 attachments, 11 obsolete attachments)

1.42 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
329.67 KB, patch
tbsaunde
: review+
davidb
: feedback+
Details | Diff | Splinter Review
16.09 KB, patch
mats
: review+
Details | Diff | Splinter Review
5.33 KB, text/html
Details
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).
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?
Yes, I think it should be calling accessibility methods directly rather than using events. Same with other areas such as menus.
(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.
Can you make a list of which notifications you need?
(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.
(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
Enn, ping please?
Severity: normal → major
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.
Enn, should DOM document be considered focusable always in means focus event can be fired for any DOM document (HTML or XUL)?
A focus event can be fired on any document and window.
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?
Target Milestone: --- → mozilla8
Version: unspecified → Trunk
Target Milestone: mozilla8 → ---
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.
Blocks: 618046
Posted patch wip (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Comment on attachment 557873 [details] [diff] [review]
wip

There's some nice cleanup here.
Blocks: 381602
Blocks: 474893
Blocks: 550338
Posted patch wip2 (obsolete) — Splinter Review
Attachment #557873 - Attachment is obsolete: true
Blocks: 376803
Posted patch wip3 (obsolete) — Splinter Review
Attachment #558515 - Attachment is obsolete: true
Blocks: 383759
Blocks: 384011
Posted patch wip4 (obsolete) — Splinter Review
Attachment #559434 - Attachment is obsolete: true
Blocks: 644452
Blocks: 665412
Depends on: 685846
Blocks: 433418
Posted patch wip5 (obsolete) — Splinter Review
encapsulate focus processing logic into accessible classes by new Widgets interface
Attachment #559824 - Attachment is obsolete: true
Blocks: 640716
Blocks: 646361
Posted patch wip6 (obsolete) — Splinter Review
Attachment #560315 - Attachment is obsolete: true
Attachment #560568 - Flags: feedback?(marco.zehe)
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.
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!
Posted patch wip7 (obsolete) — Splinter Review
build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-acc172f85d85/
Attachment #560568 - Attachment is obsolete: true
Attachment #560568 - Flags: feedback?(marco.zehe)
Attachment #560783 - Flags: feedback?(marco.zehe)
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #560783 - Attachment is obsolete: true
Attachment #560783 - Flags: feedback?(marco.zehe)
Attachment #560797 - Flags: review?(trev.saunders)
Attachment #560797 - Flags: review?(bzbarsky)
Attachment #560797 - Flags: feedback?(marco.zehe)
Posted patch patch2 (obsolete) — Splinter Review
polish Widgets interface
Attachment #560797 - Attachment is obsolete: true
Attachment #560797 - Flags: review?(trev.saunders)
Attachment #560797 - Flags: review?(bzbarsky)
Attachment #560797 - Flags: feedback?(marco.zehe)
Attachment #560856 - Flags: review?(trev.saunders)
Attachment #560856 - Flags: review?(bzbarsky)
Attachment #560856 - Flags: feedback?(marco.zehe)
latest try server build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-e0c405c742e4 (contains a small fix for random focus menu problem)
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".
Attachment #560856 - Flags: review?(bzbarsky) → review-
(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.
Posted patch dom/base partSplinter Review
Attachment #560960 - Flags: review?(bzbarsky)
Posted patch layout part (obsolete) — Splinter Review
Attachment #560961 - Flags: review?(bzbarsky)
Posted patch accessible part (obsolete) — Splinter Review
Attachment #560856 - Attachment is obsolete: true
Attachment #560856 - Flags: review?(trev.saunders)
Attachment #560856 - Flags: feedback?(marco.zehe)
Attachment #560962 - Flags: review?(trev.saunders)
Attachment #560962 - Flags: feedback?(marco.zehe)
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.
Attachment #560960 - Flags: review?(bzbarsky) → review?(enndeakin)
Attachment #560961 - Flags: review?(bzbarsky) → review?(matspal)
(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
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.
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 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).
Attachment #560962 - Flags: feedback?(marco.zehe) → feedback-
Attachment #560960 - Flags: review?(enndeakin) → review+
(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?
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.
Can you give me complete steps to reproduce then? I don't see a difference between regular build and try build.
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.
I definitely hear high "poom" sound when I tab out the search input and low "chick" when I go back.
What does turning on/off vb mean from API point of view? 

Can you see this problem on any other try server builds?
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.
(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 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 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...
Attachment #560962 - Flags: feedback- → feedback+
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!
Attachment #560962 - Flags: review+
(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!
(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.
(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!
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.
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.
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.
fixed Marco's comments and fixed a menu issue
Attachment #560962 - Attachment is obsolete: true
Attachment #560962 - Flags: review?(trev.saunders)
Attachment #561771 - Flags: review?(trev.saunders)
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 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 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.
Attachment #561771 - Flags: review?(trev.saunders) → review+
(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
fixed queryframe_target issue for nsMenuPopupFrame
Attachment #560961 - Attachment is obsolete: true
Attachment #560961 - Flags: review?(matspal)
Attachment #562388 - Flags: review?
Attachment #562388 - Flags: review? → review?(matspal)
Blocks: 646377
Mats, can you share your review plans please? This is crucial bug for accessibility and I'd love to get it in asap.
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.
What is the intended behavior when "2" is the current node?

<select multiple><option>0<option>1<option style="display:none">2<option>3
(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.
Blocks: 684818
Posted file Testcase
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
Attachment #562388 - Flags: review?(matspal) → review+
(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?
I started try server build with Mats comments fixed against trunk
Comment on attachment 561771 [details] [diff] [review]
accessible part (v2)

f=me great work.
Attachment #561771 - Flags: review+
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.
Blocks: 646596
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/30c186f6b48a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Flags: in-testsuite+
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
Status: RESOLVED → VERIFIED
Depends on: 691041
Depends on: 691580
Depends on: 691734
Blocks: 404060
Depends on: 694818
Depends on: 703198
Depends on: 701669
No longer depends on: 701669
Depends on: 717753
Depends on: 718237
You need to log in before you can comment on or make changes to this bug.