Last Comment Bug 722248 - Some chrome accessibles report incorrect visibility states
: Some chrome accessibles report incorrect visibility states
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: David Bolter [:davidb]
:
:
Mentors:
Depends on: 591363
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-30 02:11 PST by James Teh [:Jamie]
Modified: 2012-04-13 08:40 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (2.78 KB, patch)
2012-01-30 10:12 PST, David Bolter [:davidb]
no flags Details | Diff | Splinter Review
patch (2.78 KB, patch)
2012-01-30 10:36 PST, David Bolter [:davidb]
no flags Details | Diff | Splinter Review
patch (qrefreshed this time) (2.72 KB, patch)
2012-01-30 10:38 PST, David Bolter [:davidb]
no flags Details | Diff | Splinter Review
Moves ancestor visibility check to end (like we used to have). (2.67 KB, patch)
2012-01-30 11:26 PST, David Bolter [:davidb]
mzehe: review+
surkov.alexander: feedback+
Details | Diff | Splinter Review

Description James Teh [:Jamie] 2012-01-30 02:11:14 PST
Recently, some chrome accessibles are reporting the invisible and offscreen states when they are definitely visible, while others aren't reporting these states when they should. Examples:
* In the Options dialog, the OK, Cancel and Help buttons report invisible and offscreen;
* In the About dialog, many visible links and labels report invisible and offscreen;
* In the About dialog, hidden property pages and their descendants don't report invisible and offscreen;
* All accessibles for context menus report invisible and offscreen;
* The descendants of visible chrome alert dialogs report invisible and offscreen (e.g. type foo:bar into the location bar).

I'm not sure exactly when this started happening, but at a guess, it probably relates to bug 591363, as I noticed it fairly recently.
Comment 1 James Teh [:Jamie] 2012-01-30 02:16:27 PST
Not sure if this is related, but the menu accessible for context menus also always exposes the collapsed state, even though it isn't collapsed. This too wasn't the case before.
Comment 2 Marco Zehe (:MarcoZ) 2012-01-30 02:29:16 PST
Yes I've seen the context menu "collapsed" issue, too, but hadn't got around to investigating it yet. I agree that bug 591363 sounds like a very possible candidate for this breackage.
Comment 3 David Bolter [:davidb] 2012-01-30 06:13:29 PST
Quite possibly that bug yeah, and would be good to know for sure.
Comment 4 Marco Zehe (:MarcoZ) 2012-01-30 06:41:27 PST
Regression range: January 10, 2012 is OK, January 11, 2012 is broken. Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c713003d3226&tochange=e79ef0ffcb09
Comment 5 David Bolter [:davidb] 2012-01-30 10:12:56 PST
Created attachment 592765 [details] [diff] [review]
fix

All tests pass.

Manually confirmed this fixes bug (via accprobe).

Description: We no longer use the view API and that will be unavailable to us this year anyways. We no longer climb ancestors ourselves and instead call frame API to check ancestors like we did before. I've added comments regarding what we should keep an eye on.
Comment 6 David Bolter [:davidb] 2012-01-30 10:36:19 PST
Created attachment 592774 [details] [diff] [review]
patch
Comment 7 David Bolter [:davidb] 2012-01-30 10:38:19 PST
Created attachment 592776 [details] [diff] [review]
patch (qrefreshed this time)
Comment 8 David Bolter [:davidb] 2012-01-30 10:50:12 PST
We need to get these cases under test. Setting flag so we don't forget.
Comment 9 David Bolter [:davidb] 2012-01-30 11:26:14 PST
Created attachment 592790 [details] [diff] [review]
Moves ancestor visibility check to end (like we used to have).
Comment 10 David Bolter [:davidb] 2012-01-30 11:36:22 PST
Pushed to try, included a11y talos: https://hg.mozilla.org/try/rev/ea0abaa7ca3f
Comment 11 Marco Zehe (:MarcoZ) 2012-01-30 12:05:45 PST
Comment on attachment 592790 [details] [diff] [review]
Moves ancestor visibility check to end (like we used to have).

r=me test-wise. And I think from looking at where bug 591363 came from that this is the right way to fix it. Getting rid of that loop and using the frame API instead is good practice, and I also don't see performance degradation from manual testing.
Comment 12 David Bolter [:davidb] 2012-01-30 12:32:13 PST
Alexander, this is a partial revert of bug 591363. I want to get this fix (or alternative) uplifted.

All mochitests pass (including the new test from 591363). Manual testing looks good and confirms fixage.

Notes:
* I removed the direct usage of views.
* I removed the climbing of accessible and reverted to the frame ancestor check.
* There might be some perf impact since we don't early bail before the rect stuff anymore. I will monitor this.
* We could check style visibility and possibly bail before the rect stuff, but you and I have been unsure that is right (in previous discussions). Note the frame ancestor check also checks this style vis. We could move the ancestor check earlier, but I'd rather play it this way.

Ultimately I don't think we are done and have filed bug 722417.
Comment 13 David Bolter [:davidb] 2012-01-30 17:51:49 PST
I only did debug builds (silly me):
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbolter@mozilla.com-ea0abaa7ca3f/
Comment 14 alexander :surkov 2012-01-30 18:16:50 PST
Comment on attachment 592790 [details] [diff] [review]
Moves ancestor visibility check to end (like we used to have).

Review of attachment 592790 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/nsAccessible.cpp
@@ -609,5 @@
> -    const nsIView* view = frame->GetView();
> -    if (view && view->GetVisibility() == nsViewVisibility_kHide)
> -      return vstates;
> -    
> -  } while (accessible = accessible->Parent());

it doesn't worked for chrome accessible because you reach application accessible which doesn't have a frame at all.

@@ +630,5 @@
>    }
>  
> +  // XXX Do we really need to cross from content to chrome ancestor?
> +  if (!frame->IsVisibleConsideringAncestors(nsIFrame::VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY))
> +    return vstates;

it doesn't work for decks like XUL tabs.
Comment 15 alexander :surkov 2012-01-30 18:31:57 PST
Comment on attachment 592790 [details] [diff] [review]
Moves ancestor visibility check to end (like we used to have).

that's better what we have but not something we want, taking into account this patch is wanted before merging then I'm fine to take it

no review, but f+
Comment 16 David Bolter [:davidb] 2012-01-30 19:08:03 PST
Inbound land: https://hg.mozilla.org/integration/mozilla-inbound/rev/f719c5a97683
(Drat! Sorry forgot f=surkov)

Marco when you're up can you check to see if this will make the cut over?
Comment 17 Timothy Nikkel (:tnikkel) 2012-01-30 22:35:25 PST
(In reply to alexander :surkov from comment #14)
> > +  // XXX Do we really need to cross from content to chrome ancestor?
> > +  if (!frame->IsVisibleConsideringAncestors(nsIFrame::VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY))
> > +    return vstates;
> 
> it doesn't work for decks like XUL tabs.

If you just make this call stop at chrome/content boundary does that serve your needs?
Comment 18 alexander :surkov 2012-01-30 22:44:43 PST
(In reply to Timothy Nikkel (:tn) from comment #17)
> (In reply to alexander :surkov from comment #14)
> > > +  // XXX Do we really need to cross from content to chrome ancestor?
> > > +  if (!frame->IsVisibleConsideringAncestors(nsIFrame::VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY))
> > > +    return vstates;
> > 
> > it doesn't work for decks like XUL tabs.
> 
> If you just make this call stop at chrome/content boundary does that serve
> your needs?

I meant tabs like in Firefox Option dialog
Comment 19 Timothy Nikkel (:tnikkel) 2012-01-31 00:02:23 PST
(In reply to alexander :surkov from comment #18)
> I meant tabs like in Firefox Option dialog

Ok, so we want those to be visible but offscreen?
Comment 20 alexander :surkov 2012-01-31 00:05:17 PST
(In reply to Timothy Nikkel (:tn) from comment #19)
> (In reply to alexander :surkov from comment #18)
> > I meant tabs like in Firefox Option dialog
> 
> Ok, so we want those to be visible but offscreen?

yes
Comment 21 Ed Morley [:emorley] 2012-01-31 06:51:57 PST
(In reply to David Bolter [:davidb] from comment #16)
> Marco when you're up can you check to see if this will make the cut over?

Made it :-)

https://hg.mozilla.org/mozilla-central/rev/f719c5a97683
Comment 22 James Teh [:Jamie] 2012-02-01 20:44:33 PST
Verified fixed in:
* Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120201 Firefox/13.0a1
* Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120201 Thunderbird/13.0a1
Comment 23 James Teh [:Jamie] 2012-02-01 20:54:22 PST
Aside from the incorrect visibility states, the incorrect collapsed state on context menus is also gone. In addition, around the same time i noticed this bug, I noticed that position info for tabs and menus in Thunderbird was missing. This is now fixed too. I'm not sure whether this is related to this bug or not, but thought it worth noting.
Comment 24 alexander :surkov 2012-02-01 22:57:37 PST
(In reply to James Teh [:Jamie] from comment #23)
> Aside from the incorrect visibility states, the incorrect collapsed state on
> context menus is also gone. In addition, around the same time i noticed this
> bug, I noticed that position info for tabs and menus in Thunderbird was
> missing. This is now fixed too. I'm not sure whether this is related to this
> bug or not, but thought it worth noting.

confirming that state collapsed on menus and missed group info is result of invisible state. I think it's enough to have mochitests for invisible/offscreen states in bug 722417.
Comment 25 David Bolter [:davidb] 2012-04-13 08:40:46 PDT
Removing in-testsuite request based on comment 24.

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