Closed
Bug 673958
Opened 14 years ago
Closed 14 years ago
rework accessible focus handling
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
VERIFIED
FIXED
mozilla10
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 2 open bugs)
Details
(Keywords: access, Whiteboard: [inbound])
Attachments
(4 files, 11 obsolete files)
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
|
MatsPalmgren_bugz
:
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).
Assignee | ||
Comment 1•14 years ago
|
||
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•14 years ago
|
||
Yes, I think it should be calling accessibility methods directly rather than using events. Same with other areas such as menus.
Assignee | ||
Comment 3•14 years ago
|
||
(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•14 years ago
|
||
Can you make a list of which notifications you need?
Assignee | ||
Comment 5•14 years ago
|
||
(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.
Assignee | ||
Comment 6•14 years ago
|
||
(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
Assignee | ||
Comment 7•14 years ago
|
||
Enn, ping please?
Assignee | ||
Updated•14 years ago
|
Severity: normal → major
Comment 8•14 years ago
|
||
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.
Assignee | ||
Comment 9•14 years ago
|
||
Enn, should DOM document be considered focusable always in means focus event can be fired for any DOM document (HTML or XUL)?
Comment 10•14 years ago
|
||
A focus event can be fired on any document and window.
Assignee | ||
Comment 11•14 years ago
|
||
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
Updated•14 years ago
|
Target Milestone: mozilla8 → ---
Comment 12•14 years ago
|
||
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.
Assignee | ||
Comment 13•14 years ago
|
||
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Comment 14•14 years ago
|
||
Comment on attachment 557873 [details] [diff] [review]
wip
There's some nice cleanup here.
Assignee | ||
Comment 15•14 years ago
|
||
Attachment #557873 -
Attachment is obsolete: true
Assignee | ||
Comment 16•14 years ago
|
||
Attachment #558515 -
Attachment is obsolete: true
Assignee | ||
Comment 17•14 years ago
|
||
Attachment #559434 -
Attachment is obsolete: true
Assignee | ||
Comment 18•14 years ago
|
||
encapsulate focus processing logic into accessible classes by new Widgets interface
Attachment #559824 -
Attachment is obsolete: true
Assignee | ||
Comment 19•14 years ago
|
||
Assignee | ||
Comment 20•14 years ago
|
||
Attachment #560315 -
Attachment is obsolete: true
Attachment #560568 -
Flags: feedback?(marco.zehe)
Comment 21•14 years ago
|
||
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•14 years ago
|
||
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!
Assignee | ||
Comment 23•14 years ago
|
||
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)
Assignee | ||
Comment 24•14 years ago
|
||
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)
Assignee | ||
Comment 25•14 years ago
|
||
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)
Assignee | ||
Comment 26•14 years ago
|
||
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 27•14 years ago
|
||
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-
Assignee | ||
Comment 28•14 years ago
|
||
(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.
Assignee | ||
Comment 29•14 years ago
|
||
Attachment #560960 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 30•14 years ago
|
||
Attachment #560961 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 31•14 years ago
|
||
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)
![]() |
||
Comment 32•14 years ago
|
||
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.
![]() |
||
Updated•14 years ago
|
Attachment #560960 -
Flags: review?(bzbarsky) → review?(enndeakin)
![]() |
||
Updated•14 years ago
|
Attachment #560961 -
Flags: review?(bzbarsky) → review?(matspal)
Assignee | ||
Comment 33•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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-
Updated•14 years ago
|
Attachment #560960 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 37•14 years ago
|
||
(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•14 years ago
|
||
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.
Assignee | ||
Comment 39•14 years ago
|
||
Can you give me complete steps to reproduce then? I don't see a difference between regular build and try build.
Comment 40•14 years ago
|
||
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.
Assignee | ||
Comment 41•14 years ago
|
||
I definitely hear high "poom" sound when I tab out the search input and low "chick" when I go back.
Assignee | ||
Comment 42•14 years ago
|
||
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•14 years ago
|
||
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.
Assignee | ||
Comment 44•14 years ago
|
||
(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•14 years ago
|
||
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)
Assignee | ||
Comment 46•14 years ago
|
||
Comment 47•14 years ago
|
||
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 48•14 years ago
|
||
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+
Assignee | ||
Comment 49•14 years ago
|
||
(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!
Assignee | ||
Comment 50•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
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.
Assignee | ||
Comment 53•14 years ago
|
||
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•14 years ago
|
||
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.
Assignee | ||
Comment 55•14 years ago
|
||
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 56•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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+
Assignee | ||
Comment 59•14 years ago
|
||
(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
Assignee | ||
Comment 60•14 years ago
|
||
fixed queryframe_target issue for nsMenuPopupFrame
Attachment #560961 -
Attachment is obsolete: true
Attachment #560961 -
Flags: review?(matspal)
Attachment #562388 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #562388 -
Flags: review? → review?(matspal)
Assignee | ||
Comment 61•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
What is the intended behavior when "2" is the current node?
<select multiple><option>0<option>1<option style="display:none">2<option>3
Assignee | ||
Comment 64•14 years ago
|
||
(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•14 years ago
|
||
Comment 66•14 years ago
|
||
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+
Assignee | ||
Comment 67•14 years ago
|
||
(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?
Assignee | ||
Comment 68•14 years ago
|
||
I started try server build with Mats comments fixed against trunk
Comment 69•14 years ago
|
||
Comment on attachment 561771 [details] [diff] [review]
accessible part (v2)
f=me great work.
Attachment #561771 -
Flags: review+
Comment 70•14 years ago
|
||
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
Updated•14 years ago
|
Attachment #561771 -
Flags: review+ → feedback+
Assignee | ||
Comment 71•14 years ago
|
||
landed on inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/30c186f6b48a
Updated•14 years ago
|
Whiteboard: [inbound]
Comment 72•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Assignee | ||
Updated•14 years ago
|
Flags: in-testsuite+
Comment 73•14 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•