Closed Bug 630194 Opened 9 years ago Closed 9 years ago

Language sub menu no longer spoken by AT once more than one dictionary is installed

Categories

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

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla2.0b12
Tracking Status
blocking2.0 --- final+

People

(Reporter: MarcoZ, Assigned: fherrera)

References

Details

(Keywords: regression, Whiteboard: [softblocker])

Attachments

(3 files, 7 obsolete files)

This is a recent regression (window will be posted in a followup comment).


STR:
1. Install at least one additional dictionary into your profile if you only have "English (U.S.) in there right now.
2. Go to a field that has spell checking.
3. Type in something that is misspelled.
4. Arrow onto the word.
5. Hit APPLICATIONS or any other means to bring up the context menu, arrow to "Language", which is a sub menu, and open it by ENTER or RightArrow.

Expected: The newly selected menu item should be spoken by NVDA or Orca.
Actual: Silence.

6. Arrow around up and down and find that the other items are also not spoken.

Once, I got it working after selecting one of the menu items. Bringing it up again I got the items spoken, then, but this wasn't reproducible. Now, when I select one of the itmes, and bring up the menu again, it will remain silent.
This is a regression from part 4 of bug 606924. Working in January 26, 2011 nightly, broken in January 27, 2011 build, and this was the only one that was checked in in that period.

Link to attachment::
https://bugzilla.mozilla.org/attachment.cgi?id=504402
Blocks: 606924
Are we ok on other submenus?
submenus on the main menubar works for me.
I don't see any issue in the sub menus of the main menu. So opening the View menu, for example, and arrowing into the Toolbars sub menu will speak the menus.

However, clicking the Firefox/Minefield button, none of the menu items are spoken.
Additional info, this seems to be an issue with Orca and NVDA, as confirmed by Fer on IRC, but NOT with JAWS. JAWS sees these menu items, or triggers proper caching for some reason.
When more than one language is installed, we are firing the focus event on the wrong object, "[menu | Languages]", that is the menu object, instead of the submenu items.
(In reply to comment #6)
> When more than one language is installed, we are firing the focus event on the
> wrong object, "[menu | Languages]", that is the menu object, instead of the
> submenu items.

Because there's no accessibles for submenu items I assume.
No, when I have only one dictionary, I get:

[...]

focus:(0, 0, None)
	source: [radio menu item | English / United States]
	host_application: [application | Minefield]
object:state-changed:focused(1, 0, None)
	source: [radio menu item | English / United States]
	host_application: [application | Minefield]
object:state-changed:iconified(0, 0, None)
	source: [window | ]
	host_application: [application | Minefield]
focus:(0, 0, None)
	source: [menu item | Add Dictionaries…]
	host_application: [application | Minefield]
object:state-changed:focused(1, 0, None)
	source: [menu item | Add Dictionaries…]
	host_application: [application | Minefield]
[...]
(In reply to comment #6)
> When more than one language is installed, we are firing the focus event on the
> wrong object, "[menu | Languages]", that is the menu object, instead of the
> submenu items.

I can't confirm this. I get focus events for sub menuitems. The tree is correct. When I select Language menu then I handle focus event for language, the tree is:

menu - Language
  menupopup - Language
    menuitem - English / United States

when I press right arrow, I get focus event for menuitem English. But NVDA keeps silent.

The tree looks the same and the same events I handle when I navigate through View->Toolbars menu where NVDA works fine.
What is the state of the object that gets focus after you press right arrow? (I like accprobe for this)
Here both objects ([menu | Languages] and [radio menu item | English / United States]) have the "focused" state.
Perhaps, Jamie, should look at this.
blocking2.0: ? → final+
Whiteboard: [softblocker]
I can confirm via accprobe I don't get focus events for the sub-menuitems from the Firefox/Minefield button. Each time I mouse hover over a sub-menuitem I get a focus event for the menu itself.
Assignee: nobody → surkov.alexander
Alexander, if this is still a bug tomorrow I can dig in, but for now I'm giving it to you as you've started (a little).
This happens to me only the first time I browse that menu. If I close submenu and menu with 2 escape presses or clicking outside and repeat the same, I get proper focus events.
It looks like a problem with the cached accessible tree.

At nsRootAccessible::HandleEvent
  origTargetNode is menuitem@id='spell-check-dictionary-en-US'

after that, at nsRootAccessible::ProcessDOMEvent
accessible = 
    GetAccService()->GetAccessibleOrContainer(origTargetNode, weakShell);

accessible Node is menu@id='spell-dictionaries'
While your observations are different than my ones (comment #6, comment #16 vs comment #9) I think the problem you are seeing can be explained/fixed well/easy. We handle DOM event for menu (or menuitmes) and process it before we've got notification from layout that menu was changed (and update the accessible tree). All we need to fix that I think is to process this DOM event as pending notification always.
Fernando, wanna take it and try the fix?
Jamie, when you get a focus event for the accessible that is not in vb, then is it a problem? When the focus event can be ignored by NVDA?
(In reply to comment #19)
> Jamie, when you get a focus event for the accessible that is not in vb, then is
> it a problem? When the focus event can be ignored by NVDA?
I'm not sure I understand. We don't use virtual buffers for this menu, since it is part of the chrome. We only use virtual buffers for read-only documents.
Ok, you're right, the first question is not valid. Do you have any ideas how to explain observations from comment #9?
Could it be that we get content insertion notification from layout for those submenu items while we are still processing the previous ones and then they got queued to be processes in the next refresh? This is some debug output that make me think that:

firefox "data:text/html,<textarea>thgus is worng</textarea>"  | grep -i spell

--- Open the context menu for the fist time ---
content removed: menuitem@id='spell-add-dictionaries-main', container: menupopup@id='contentAreaContextMenu'
content inserted: menuitem@id='spell-add-dictionaries-main', container: menupopup@id='contentAreaContextMenu', end node: 0xb09bdf80
content removed: menuitem@id='spell-no-suggestions', container: menupopup@id='contentAreaContextMenu'
content inserted: menuitem@id='spell-no-suggestions', container: menupopup@id='contentAreaContextMenu', end node: 0xa5d3bf80
pending content insertion: menuitem@id='spell-add-dictionaries-main', container: menupopup@id='contentAreaContextMenu', inserted content amount: 1
pending content insertion: menuitem@id='spell-no-suggestions', container: menupopup@id='contentAreaContextMenu', inserted content amount: 1

--- Close the context menu ---

--- Open the context menu for the second time ---
content removed: menuitem@id='spell-check-dictionary-en-US', container: menupopup@id='spell-dictionaries-menu'
content removed: menuitem@id='spell-check-dictionary-en-GB', container: menupopup@id='spell-dictionaries-menu'
content removed: menuitem@id='spell-check-dictionary-es-ES', container: menupopup@id='spell-dictionaries-menu'
content inserted: menuitem@id='spell-check-dictionary-en-US', container: menupopup@id='spell-dictionaries-menu', end node: 0xb09c7080
content inserted: menuitem@id='spell-check-dictionary-en-GB', container: menupopup@id='spell-dictionaries-menu', end node: 0xb09c7080
content inserted: menuitem@id='spell-check-dictionary-es-ES', container: menupopup@id='spell-dictionaries-menu', end node: 0xb09c7080
pending content insertion: menuitem@id='spell-check-dictionary-en-US', container: menu@id='spell-dictionaries', inserted content amount: 1
pending content insertion: menuitem@id='spell-check-dictionary-en-GB', container: menu@id='spell-dictionaries', inserted content amount: 1
pending content insertion: menuitem@id='spell-check-dictionary-es-ES', container: menu@id='spell-dictionaries', inserted content amount: 1
(In reply to comment #22)
> Could it be that we get content insertion notification from layout for those
> submenu items while we are still processing the previous ones and then they got
> queued to be processes in the next refresh?

Sure. Though layout is usually smart and reports the top of changed subtree.
But if the sub menu is built dynamically when the menu is opened? Because it may have different items depending on how many languages are installed. And it may be that since the last time the menu was accessed and this particular menu was made visible, new dictionaries were installed, so this is being built dynamically, not statically.
(In reply to comment #21)
> Ok, you're right, the first question is not valid. Do you have any ideas how to
> explain observations from comment #9?

Jamie, ignore this.
(In reply to comment #24)
> But if the sub menu is built dynamically when the menu is opened?

I think it's built dynamically, but we built accessible tree dynamically too. The question is where we fail.
Attached file debug log (obsolete) —
This is a debug output of ff showing the context menu for the first time.
It includes a log of every insertion reported from nsCSSFrameConstructor at nsAccessibilityService::ContentRangeInserted and at NotificationController::ContentInsertion the elements that are added to mInsertedContent and those that are not. 

Notice that a lot of elements are not added in a call to ContentInsertion because they are aEndChildNode, but they are added on other calls. There are the elements that never got added:
menuitem@id='spell-add-to-dictionary'
menu@id='spell-dictionaries'
menuitem@id='context-undo'
menuseparator@id='spell-separator'
The commit that broke this is:
http://hg.mozilla.org/mozilla-central/rev/f0f5638a8f51

I've dump the same log as in comment 27 with a build from the the previous  revision (where submenu items got the focus correctly) and the ContentRangeInserted / ContentInsertion behavior is identical so that is not the problem.
So then it looks like submenu children are not cached on the tree when inserted. With the attached patch, the focus event is fired for the proper items as they are in the cache.
(In reply to comment #29)

> So then it looks like submenu children are not cached on the tree when
> inserted.

this is correct observation

> With the attached patch, the focus event is fired for the proper
> items as they are in the cache.

needs a right patch that fix the problem
Boris, we don't get content range inserted notification from nsCSSFrameConstructor::ContentAppended/ContentRangeInserted for certain menuitems of context menu as per comment #27:

There are
the elements that never got added:
menuitem@id='spell-add-to-dictionary'
menu@id='spell-dictionaries'
menuitem@id='context-undo'
menuseparator@id='spell-separator'

Accessibility doesn't force frames creation for menuitems of context menu so I'm pretty sure frames are created when context menu is shown.

Can you think of a reason why we don't get notified from layout? Can nsCSSFrameConstructor construct frames for content that doesn't belong to interval [aStartChild, aEndChild)? Are there a cases when aEndChild is inclusive?
This should be a hardblocker:
1) screen reader users can't use spellcheck when text is multilanguage
2) the certain parts of UI (perhaps webpage as well) may be inaccessible
So this is a log on top of nsCSSFrameConstructor::ContentRangeInserted. 
When "spell-check-dictionary-en-US" is inserted onto menupopup@id='spell-dictionaries-menu' we hit this code path:

    nsIFrame* insertionPoint;
    GetInsertionPoint(parentFrame, aStartChild, &insertionPoint);
    if (! insertionPoint)
      return NS_OK; // Don't build the frames.

as there is no insertionPoint at that time a11y is note notified. Then opening the Languages submenu does not generate any nsCSSFrameConstructor::ContentRangeInserted. When closing the menu and opening again the context menu, we get:


nsCSSFrameConstructor::ContentRangeInserted container: menupopup@id='contentAreaContextMenu'  start-child:menuitem@id=''  end-child:menuitem@id=''   lazy=1
nsCSSFrameConstructor::ContentRangeInserted container: menupopup@id='contentAreaContextMenu'  start-child:menuitem@id=''  end-child:menuitem@id=''   lazy=1
nsCSSFrameConstructor::ContentRangeInserted container: menupopup@id='contentAreaContextMenu'  start-child:menuitem@id=''  end-child:menuitem@id=''   lazy=1
nsCSSFrameConstructor::ContentRangeInserted container: menupopup@id='spell-dictionaries-menu'  start-child:menuitem@id='spell-check-dictionary-en-US'  end-child:menuseparator@id='spell-check-dictionary-en-US'   lazy=1
nsCSSFrameConstructor::ContentRangeInserted container: menupopup@id='spell-dictionaries-menu'  start-child:menuitem@id='spell-check-dictionary-en-GB'  end-child:menuseparator@id='spell-check-dictionary-en-GB'   lazy=1
nsCSSFrameConstructor::ContentRangeInserted container: menupopup@id='spell-dictionaries-menu'  start-child:menuitem@id='spell-check-dictionary-es-ES'  end-child:menuseparator@id='spell-check-dictionary-es-ES'   lazy=1

at that point there is insertionPoint so we get proper notifications.
Attachment #510618 - Attachment is obsolete: true
> as there is no insertionPoint at that time a11y is note notified.

Which means the parent has no frame yet.

> Then opening the Languages submenu does not generate any
> nsCSSFrameConstructor::ContentRangeInserted.

Does that go through nsCSSFrameConstructor::GenerateChildFrames?  That seems to be something used by only XUL popups....
(In reply to comment #34)

> Does that go through nsCSSFrameConstructor::GenerateChildFrames?  That seems to
> be something used by only XUL popups....

yes, just after all ContentRangeInserted/ContentRemoved/StyleChangeReflow calls:

nsCSSFrameConstructor::GenerateChildFrames for frame contents: menupopup@id='spell-dictionaries-menu'
OK. We should probably add an a11y notification in GenerateChildFrames.
Assignee: surkov.alexander → fherrera
Attachment #510716 - Attachment is obsolete: true
Attachment #511120 - Flags: superreview?(bzbarsky)
Attachment #511120 - Flags: review?(surkov.alexander)
Is there a reason to not just send a ContentRangeInserted notification here?
(In reply to comment #38)
> Is there a reason to not just send a ContentRangeInserted notification here?

ContentRangeInserted seems right to me.
Attachment #511120 - Attachment is obsolete: true
Attachment #511143 - Flags: superreview?(bzbarsky)
Attachment #511143 - Flags: review?(surkov.alexander)
Attachment #511120 - Flags: superreview?(bzbarsky)
Attachment #511120 - Flags: review?(surkov.alexander)
Comment on attachment 511143 [details] [diff] [review]
Updated patch using ContentRangeInserted

sr=me
Attachment #511143 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 511143 [details] [diff] [review]
Updated patch using ContentRangeInserted

Fernando, please add a comment we handle poupups here if Boris is ok.

We must have mochitest on this. It could look like:

<menupopup id="context">
  <menuitem label="item1"/>
  <menuitem label="item2"/>
  <menu id="item3">
    <menupopup>
      <menu label="item3.1">
        <menupopup>
          <menuitem label="item3.1.1"/>
        </menupopup>
      </menu>
    </menu>
  </menu>
</menupopup>

<button context="context">btn</button>

we don't force frames generation of children for top menupopup so we don't create accessibles on initial accessible creation and we can watch for show events for menuitems. Please make sure show events for submenuitem are coalesced with its parent.
Attachment #511143 - Flags: review?(surkov.alexander) → review+
Flags: in-testsuite?
I can confirm that the try-server build fixes the problem!
Attached patch mochitest for this bug (obsolete) — Splinter Review
The test only checks for focus events on the items. 
EVENT_MENUPOPUP_START / END events are commented out on open/close submenu tests because of bug #617311 on Linux.

Are they fired on Win/Mac? Should we conditionally listen for then? Just skipping?
Attachment #511363 - Flags: review?(surkov.alexander)
(In reply to comment #45)

> Are they fired on Win/Mac? Should we conditionally listen for then? Just
> skipping?

they should, we can skip them. I'd like to see testing of accessible when menus gets open.

if you're going to add test the tree then move it to treeupdate folder and give a proper name, otherwise it should be in events/test_focus_menus.html or something.
Attachment #511143 - Attachment is obsolete: true
Attached patch updated mochitest (obsolete) — Splinter Review
Attachment #511363 - Attachment is obsolete: true
Attachment #511363 - Flags: review?(surkov.alexander)
Attachment #511742 - Flags: review?(surkov.alexander)
Comment on attachment 511741 [details] [diff] [review]
updated patch with comment

let's get Boris's feedback on comment.

(well, it sounds not very clear for me).
Attachment #511741 - Flags: feedback?(bzbarsky)
Comment on attachment 511741 [details] [diff] [review]
updated patch with comment

How about:

  Now that we have created frames for these children, notify accessibility about
  them.

Or do you really need to mention popups for some reason?
Attachment #511741 - Flags: feedback?(bzbarsky) → feedback-
(In reply to comment #50)

> Or do you really need to mention popups for some reason?

I'd like to see a reason why we need to notify a11y from here, i.e. why it doesn't handled by ContentRangeInserted.
Well, because clearly ContentRangeInserted didn't create these frames!  If it did, calling this function would be a very bad thing, no?
Put another way, by the time you're calling this function you don't know _why_ these frames don't exist; just that they don't and they need to.  Correct?
Attached patch updated mochitest (obsolete) — Splinter Review
I have updated the test to use testAccessibleTree when opening the menu. Maybe we should remove the FOCUS checks as a failure about not having the proper accessible tree is more clear failure than missing a focus event. What do you think Alex?
Attachment #511742 - Attachment is obsolete: true
Attachment #511785 - Flags: review?(surkov.alexander)
Attachment #511742 - Flags: review?(surkov.alexander)
(In reply to comment #52)
> Well, because clearly ContentRangeInserted didn't create these frames!

absolutely :) I just would like to see something that describes when it works.

>  If it
> did, calling this function would be a very bad thing, no?

not very bad thing in terms of something gets broken, but obviously excess work and perf issue.

(In reply to comment #53)
> Put another way, by the time you're calling this function you don't know _why_
> these frames don't exist; just that they don't and they need to.  Correct?

correct. So can I say this method is a fallback when ContentRangeInserted doesn't work due to some reason and it doesn't trigger when ContentRangeInserted works?
(In reply to comment #54)
> Created attachment 511785 [details] [diff] [review]
> updated mochitest
> 
> I have updated the test to use testAccessibleTree when opening the menu. Maybe
> we should remove the FOCUS checks as a failure about not having the proper
> accessible tree is more clear failure than missing a focus event. What do you
> think Alex?

This test 2 in 1 what is not bad, let's keep it complex as is.

few nits:

Accessible menu events testing for XUL menu
-> menu tree and events

var contextTree = {
-> gContextTree
Comment on attachment 511785 [details] [diff] [review]
updated mochitest


>+    function openSubMenu(submenuID, itemID)
>+    {
>+      this.eventSeq = [
>+        new invokerChecker(EVENT_FOCUS, getNode(itemID)),
>+      ];
>+
>+      this.invoke = function openMenu_invoke()

openSubMenu_invoke

>+      this.getID = function openMenu_getID()

the same

>+    function closeSubMenu(submenuID, itemID)
>+    {
>+      this.eventSeq = [
>+        new invokerChecker(EVENT_FOCUS, getNode(itemID)),
>+      ];
>+
>+      this.invoke = function openMenu_invoke()
>+      {

closeSubMenu

>+
>+      this.getID = function openMenu_getID()

the same

>+    function closeMenu(aID)
>+    {
>+
>+      this.invoke = function openMenu_invoke()

^
+      }
>+
>+      this.getID = function openMenu_getID()

^

>+        {
>+          name: "item2",
>+          role: ROLE_PARENT_MENUITEM,
>+          children: [
>+            {
>+              name: "item2.0",
>+              role: ROLE_PARENT_MENUITEM,
>+              children: [

this should fail on windows, since there's menupopup accessible in the middle, we should have different trees depending on platform I think.


>+                { 
>+                  name: "item2.0.0",
>+                  role: ROLE_MENUITEM,
>+                  children: []
>+                }
>+              ]
>+            }
>+          ]
>+        }
>+      ]
>+    };
>+
>+    function doTests()
>+    {
>+      gQueue = new eventQueue();
>+
>+      // Check initial empty tree
>+      testAccessibleTree("context", { role: ROLE_MENUPOPUP, children: [] });

you can use short version 
testAccessibleTree("context", { MENUPOPUP: [ ] });

>+
>+  <hbox flex="1" style="overflow: auto;">
>+  <body xmlns="http://www.w3.org/1999/xhtml">
>+    <a target="_blank"
>+       href="https://bugzilla.mozilla.org/show_bug.cgi?id=630194"
>+       title="a11y not notified on items inserted when parent frame does not exist yes">

perhaps title should be so much layout :)
Attachment #511785 - Flags: review?(surkov.alexander)
> I just would like to see something that describes when it works.

I'm not sure what you mean.

> not very bad thing in terms of something gets broken

Uh... I wasn't talking about your a11y function.  If someone calls GenerateChildFrames on a node whose kids already have frames we will almost certainly crash.  If we're lucky, frame poisoning will keep that from being exploitable.  So if you're in this code, you know no frames have been created for the kids before this.

> So can I say this method is a fallback when ContentRangeInserted
> doesn't work due to some reason and it doesn't trigger when
> ContentRangeInserted works?

It's not a fallback.  It's not "some reason".  Some frames purposefully suppress descendant creation and then lazily do it via this API.  At the moment they all happen to be popups, but that's not set in stone.

Again, it seems like you just need to let a11y know when frames got created.  If this method is called, that means frames got created, ergo a11y needs to be notified.  Why does the comment need to go into more detail than that (esp. since pretty much any such detail would be wrong)?

>+       title="a11y not notified on items inserted when parent frame does not exist yes">

s/yes/yet/?  And that's wrong as a general title, btw; in general a11y is notified on such things when their parent frame gets created.  There are just a few cases that are special where that doesn't happen.
(In reply to comment #58)
> > I just would like to see something that describes when it works.
> 
> I'm not sure what you mean.

Perhaps this detail belongs as a line in the commit message? Maybe "We added the content insertion call into a11y here to make this work for popups, but it should generally cover cases were descendant frame creation is suppressed/lazy" or something.

> >+       title="a11y not notified on items inserted when parent frame does not exist yes">
> 
> s/yes/yet/?  And that's wrong as a general title, btw; in general a11y is
> notified on such things when their parent frame gets created.  There are just a
> few cases that are special where that doesn't happen.

How about: "Make sure accessibility is notified for all lazily generated frames"?
I did a conditional if (LINUX || SOLARIS) to define the tree without the popups or with them. I am not sure about MAC here. Will push a try server run.
Attachment #511785 - Attachment is obsolete: true
Attachment #511801 - Flags: review?(surkov.alexander)
> Perhaps this detail belongs as a line in the commit message?

Yes, that makes a lot more sense.

> "Make sure accessibility is notified for all lazily generated frames"

Sounds good to me.
Comment on attachment 511801 [details] [diff] [review]
updated mochitest


>+    
>+    function openSubMenu(submenuID, itemID)

-> aSubMenuID, aItemID

>+    function closeSubMenu(submenuID, itemID)

the same

>+    <a target="_blank"
>+       href="https://bugzilla.mozilla.org/show_bug.cgi?id=630194"
>+       title="accessible tree updated when opening menu popup">

perhaps: Update accessible tree when opening the menu popup.

I'll fix these locally
Attachment #511801 - Flags: review?(surkov.alexander) → review+
landed on 2.0 (fx4b12)
patch: http://hg.mozilla.org/mozilla-central/rev/7fa4835d58bd
mochitest: http://hg.mozilla.org/mozilla-central/rev/393fbb42cfe3

I added a comment as commit message, no comment in the code.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110215 Firefox/4.0b12pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.