Last Comment Bug 677154 - Detached document accessibility tree
: Detached document accessibility tree
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Windows 7
: P1 major (vote)
: mozilla13
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: treea11y
  Show dependency treegraph
 
Reported: 2011-08-07 23:37 PDT by James Teh [:Jamie]
Modified: 2012-02-27 00:51 PST (History)
6 users (show)
mzehe: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
fixed
affected


Attachments
patch (999 bytes, patch)
2012-02-15 20:51 PST, alexander :surkov
bzbarsky: review+
Details | Diff | Splinter Review
patch2 (6.36 KB, patch)
2012-02-17 06:27 PST, alexander :surkov
mzehe: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description James Teh [:Jamie] 2011-08-07 23:37:00 PDT
Set up steps (only need to be done once):
1. Go to http://www.pennytel.com/.
2. Sign up for a "Personal VoIP-Free Access" account. You should only need to complete the first screen. You can just close the window requesting address details, etc.
3. Visit https://www.pennytel.com/login.jsp and log in.
4. Disable the soft phone to make life easier and the test simpler.
5. Log out.

Str:
1. With NVDA running, visit https://www.pennytel.com/login.jsp and log in.
2. Select "Profile" from the main navigation bar.
3. Retrieve and examine the tree of the accessible for the document in the last iframe (where the main content of the page is presented).
Expected: There should be quite a lot of accessibles, starting with a form just below the document.
Actual: There is just a "text frame" accessible. The tag attribute reveals this to be for the "body" tag.
Note: If this doesn't occur, repeat from step 2. It doesn't appear to be 100% reproduceable, but it does happen more often than not for me.
4. Press tab a few times to focus inside this document tree.
5. Retrieve the accessible for the focus.
6. Walk up its parents using IAccessibleObject::accParent to find the document accessible.
Expected: You should eventually reach the document accessible.
Actual: When you call IAccessible::accParent on the form accessible, you get E_UNEXPECTED.
Comment 1 alexander :surkov 2011-08-09 03:47:44 PDT
I did steps 1-3 (str).

The tree I see:

document "Welcome to PenyTel :: User Interface"
 table
  row
   cell
    section
     section (has children)
     section
      internal frame
       document "Welcome to PenyTel :: User Interface"
        text container (HTML body)
         form (HTML form)
          table (has children)

Am I looking at correct document? I'm running NVDA 2011.2b3.
Comment 2 James Teh [:Jamie] 2011-08-09 05:15:09 PDT
My bad. This doesn't appear to work on newly created accounts. :(
Comment 3 James Teh [:Jamie] 2011-09-23 06:18:05 PDT
This document seems to cause the same problem, but it seems to be 100% reproduceable for me and is much simpler to reproduce as no login is needed:
http://www.eveready.com/batteries/Pages/eveready-gold.aspx
Tested with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110917 Firefox/9.0a1
Will test with updated build later, but wanted to note the URL before I lose it.
Comment 4 Marco Zehe (:MarcoZ) 2011-09-23 09:26:26 PDT
I can definitely reproduce this in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110922 Firefox/9.0a1.
Comment 5 alexander :surkov 2011-09-26 04:17:18 PDT
(In reply to James Teh [:Jamie] from comment #3)
> This document seems to cause the same problem, but it seems to be 100%
> reproduceable for me and is much simpler to reproduce as no login is needed:
> http://www.eveready.com/batteries/Pages/eveready-gold.aspx
> Tested with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110917
> Firefox/9.0a1
> Will test with updated build later, but wanted to note the URL before I lose
> it.

Can you give me steps to reproduce? I don't see any iframe there and missed content.
Comment 6 Marco Zehe (:MarcoZ) 2011-09-26 04:35:35 PDT
Just open the page Jamie points to. I only see two skip to... links in the NVDA virtual buffer. The rest of the page is NOT there. But if I tab through the page, NVDA reports focus on elements that i don't find in the accessibility tree.
Comment 7 James Teh [:Jamie] 2011-09-26 04:43:24 PDT
(In reply to alexander surkov from comment #5)
> > http://www.eveready.com/batteries/Pages/eveready-gold.aspx
> Can you give me steps to reproduce? I don't see any iframe there and missed
> content.
Sorry for not providing more thorough info. I was in a hurry, but wanted to note the URL.

There's no iframe, but there is definitely a great deal of missed content.

Str:
1. With NVDA running, visit http://www.eveready.com/batteries/Pages/eveready-gold.aspx
2. Tab to the "HOME" link.
3. Retrieve the accessible for the focus.
4. Walk up its parents using IAccessibleObject::accParent to find the document accessible.
Expected: You should eventually reach the document accessible.
Actual: When you call IAccessible::accParent on the accessible for the div with @id="s4-bodyContainer", you get E_UNEXPECTED.
Comment 8 alexander :surkov 2011-09-26 21:18:29 PDT
Hm, can't reproduce it. get_accParent can return E_UNEXPECTED iff when there's no Gecko parent. So I listed gecko's parent chain when Gecko focus event is handled and it looks ok when for "HOME" link.
Comment 9 James Teh [:Jamie] 2011-09-26 21:47:04 PDT
This is really odd. I can't reproduce this with a clean profile. However, with my normal profile, it still happens even in safe mode. Marco, can you reproduce this with a clean profile?

This suggests that some pref or setting must be affecting this, but I have no idea what.
Comment 10 James Teh [:Jamie] 2012-01-24 23:06:42 PST
Uh oh. Yet another case of this reported by an NVDA user:
http://www.wssdemo.com/default.aspx
On this page, nothing past the toolbar is attached to the accessibility tree. Again, frustratingly, if I use a clean profile, I can't reproduce it, but with my normal profile (even with safe mode), it happens every time. I don't know what's causing this, but it is affecting multiple users.

Str:
1. With NVDA running, visit the URL above.
2. Move focus to the top of the document by pressing control+k then tab.
3. Press NVDA+space to force focus mode so that NVDA doesn't intercept the tab key.
4. Press tab 11 times.
5. Retrieve the accessible for the focus.
6. Walk up its parents using IAccessible::accParent to find the document accessible.
Expected: You should eventually reach the document accessible.
Actual: When you call IAccessible::accParent on the accessible for the div with @id="s4-bodyContainer", you get E_UNEXPECTED.

Related NVDA ticket: http://www.nvda-project.org/ticket/2059
Comment 11 alexander :surkov 2012-01-25 20:46:29 PST
Jamie, maybe you could share your profile with us (sending it by email excluding your private data like passwords)?
Comment 12 James Teh [:Jamie] 2012-01-31 14:30:29 PST
I've shared a profile privately with which I can reproduce the issue.
Comment 13 Marco Zehe (:MarcoZ) 2012-02-08 22:35:46 PST
This bug is now being exposed in the latest layout of Gmail if standard settings are active and for example letters j and k work to move from message to message. Bumping priority and severity accordingly.
Comment 14 alexander :surkov 2012-02-09 00:44:18 PST
(In reply to Marco Zehe (:MarcoZ) from comment #6)
> Just open the page Jamie points to. I only see two skip to... links in the
> NVDA virtual buffer. The rest of the page is NOT there. But if I tab through
> the page, NVDA reports focus on elements that i don't find in the
> accessibility tree.

I can reproduce this part when Firefox window is maximized (thanks to Jamie's profile), otherwise the page works fine. But I can't reproduce Jamie's comment #7:

(In reply to James Teh [:Jamie] from comment #7)
> Str:
> 1. With NVDA running, visit
> http://www.eveready.com/batteries/Pages/eveready-gold.aspx
> 2. Tab to the "HOME" link.
> 3. Retrieve the accessible for the focus.
> 4. Walk up its parents using IAccessibleObject::accParent to find the
> document accessible.
> Expected: You should eventually reach the document accessible.
> Actual: When you call IAccessible::accParent on the accessible for the div
> with @id="s4-bodyContainer", you get E_UNEXPECTED.

I tried by DOMi to listen focus event and print parents chain (crossplatform part works). I tried to inspect IAccessible::accParent on div@id="s4-bodyContainer" by accprobe and it returns a parent. Is the magic that I should get IAccessible::accParent when I handle MSAA focus event?
Comment 15 alexander :surkov 2012-02-09 00:47:58 PST
Hm, you know, I can see incomplete tree (div@id="s4-bodyContainer" part is missed), maybe I tried accParent after the tree was magically updated (but not NVDA vb). I'll look into this more.
Comment 16 alexander :surkov 2012-02-09 21:39:47 PST
div@id="s4-workspace" that contains div@id="s4-bodyContainer" is not accessible initially. So initially HTML form accessible contains div@id="s4-bodyContainer" accessible as a child. Eventually div@id="s4-workspace" gets an accessible eventually and the accessible is created when HTML form accessible children are updated. Since that update is not for div@id="s4-workspace" then we don't cache children for it, therefore div@id="s4-bodyContainer" subtree gets unattached.

div@id="s4-workspace" has scroll frame and its accessibility status depends whether the scrollframe is focusable or not. The problem is focusable state can be changed during frame lifecycle.

This part of code (http://hg.mozilla.org/mozilla-central/diff/bd1754632d8c/layout/generic/nsFrame.cpp) defines focusable state for scrollframe. When initial accessible tree is created then scrollFrame->GetScrollRange() is empty what makes it inaccessible. After sometime scrollrange gets unempty so we create an accessible as described above.

I tempted to say this is regression from bug 679791 (targeted to Mozilla 9) which changed focuability logic for scroll frames but bug 679791 was fixed 17 august but this one was reported 07 Aug. Anyway cc'ing Roc and Josh for help. 

The fix could be: notify accessibility when result of nsIFrame::IsFocusable is changed during frame lifecycle.
Comment 17 alexander :surkov 2012-02-15 20:51:27 PST
Created attachment 597680 [details] [diff] [review]
patch

Since result !scrollFrame->GetScrollRange().IsEqualEdges(nsRect(0, 0, 0, 0)) of nsIFrame::IsFocusable (http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#7206) can be changed during frame life cycle without any notification to a11y then IsFocusable() check doesn't work for us (http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#972).

So at minimum we can create an accessible for nsHTMLScrollFrame if it's not root of native anonymous subtree (port from nsIFrame::IsFocusable), at least I didn't noticed accessible tree changes in general.

If the result of scrollFrame->GetScrollbarStyles() != nsIScrollableFrame::ScrollbarStyles(NS_STYLE_OVERFLOW_HIDDEN, NS_STYLE_OVERFLOW_HIDDEN) cannot be changed during frame life cycle then it's worth to add this check too. If it can be changed then alternatively we could listen style changes and notify a11y.

But in general I'm not sure what nsHTMLScrollFrame is used for, i.e. which HTML elements have this frame and under which circumstances. Knowing this could help me to answer whether we can keep it accessible always or update accessible tree when for example overflow style is changed.

Also, I failed to create a testcase when result of scrollFrame->GetScrollRange().IsEqualEdges() is changed during frame life cycle. 

Boris, that'd be great if you can help me to find answers.
Comment 18 Boris Zbarsky [:bz] (TPAC) 2012-02-16 09:54:40 PST
> But in general I'm not sure what nsHTMLScrollFrame is used for

Scrolling.  Basically any time you have a block with overflow:scroll or overflow:auto.  Also used for the viewport scrollbars.

I _think_ that the return value of GetScrollbarStyles() can't change without reframing, but check with roc?

> Also, I failed to create a testcase when result of
> scrollFrame->GetScrollRange().IsEqualEdges() is changed during frame life cycle. 

Any testcase that dynamically changes the height of the content inside an overflow:scroll block should to it.
Comment 19 Boris Zbarsky [:bz] (TPAC) 2012-02-16 09:56:45 PST
Comment on attachment 597680 [details] [diff] [review]
patch

"regardless of"
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-16 14:37:16 PST
(In reply to Boris Zbarsky (:bz) from comment #18)
> I _think_ that the return value of GetScrollbarStyles() can't change without
> reframing, but check with roc?

I think that's right.
Comment 21 alexander :surkov 2012-02-17 06:27:10 PST
Created attachment 598222 [details] [diff] [review]
patch2

added scrollbar styles check
added mochitests
Comment 22 Marco Zehe (:MarcoZ) 2012-02-17 06:51:12 PST
Comment on attachment 598222 [details] [diff] [review]
patch2

r=me for the tests.
Comment 23 alexander :surkov 2012-02-17 09:37:26 PST
inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/4c23b6fefdb2
Comment 24 alexander :surkov 2012-02-17 09:48:27 PST
forgot to fix misspelling (comment #19) https://hg.mozilla.org/integration/mozilla-inbound/rev/c4488d89ae97
Comment 26 alexander :surkov 2012-02-17 18:01:07 PST
Jamie, could you please check other examples if they were fixed too?
Comment 27 Marco Zehe (:MarcoZ) 2012-02-20 01:35:14 PST
I can verify that gmail is fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/13.0a1 Firefox/13.0a1
Comment 28 Marco Zehe (:MarcoZ) 2012-02-20 01:43:11 PST
Alex, is this a regression from the big frames rework of Firefox 4 days? Can we justify getting this approved for Aurora and Beta?
Comment 29 alexander :surkov 2012-02-20 02:06:07 PST
(In reply to Marco Zehe (:MarcoZ) from comment #28)
> Alex, is this a regression from the big frames rework of Firefox 4 days?

I think changes in layout can tweak bug reproducibility. But yes, Firefox 4 accessible tree rework is a root cause.

> Can
> we justify getting this approved for Aurora and Beta?

we should do this. 

from technical side: this patch shouldn't deliver any regressions because all it can do is to alter an accessible tree the way screen readers don't care. In 99% no change in tree, without the patch certain web sites accessibility is dead that's a reason why it's important to port it.
Comment 30 alexander :surkov 2012-02-20 05:04:43 PST
Comment on attachment 598222 [details] [diff] [review]
patch2

[Approval Request Comment]
Regression caused by (bug #): not known
User impact if declined: certain web cites are not accessible
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: no
Comment 31 Alex Keybl [:akeybl] 2012-02-21 11:27:37 PST
Comment on attachment 598222 [details] [diff] [review]
patch2

[Triage Comment]
This is low risk and in support of accessibility - approved for Aurora 12.
Comment 32 Marco Zehe (:MarcoZ) 2012-02-22 03:37:59 PST
Landed on Aurora at Surkov's request: http://hg.mozilla.org/releases/mozilla-aurora/rev/c2749c1ced57
Comment 33 James Teh [:Jamie] 2012-02-27 00:51:13 PST
All other test cases verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120226 Firefox/13.0a1

Note You need to log in before you can comment on or make changes to this bug.