722248 - Some chrome accessibles report incorrect visibility states

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

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]]

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)]

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] (NeedInfo me for attention)]

Quite possibly that bug yeah, and would be good to know for sure.

Comment 4

[Marco Zehe (:MarcoZ)]

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] (NeedInfo me for attention)]

Attachment: 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] (NeedInfo me for attention)]

Attachment: patch

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch (qrefreshed this time)

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

We need to get these cases under test. Setting flag so we don't forget.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: Moves ancestor visibility check to end (like we used to have).

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Pushed to try, included a11y talos: https://hg.mozilla.org/try/rev/ea0abaa7ca3f

Comment 11

[Marco Zehe (:MarcoZ)]

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] (NeedInfo me for attention)]

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] (NeedInfo me for attention)]

I only did debug builds (silly me):
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ea0abaa7ca3f/

Comment 14

[alexander :surkov (:asurkov)]

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 (:asurkov)]

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] (NeedInfo me for attention)]

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)]

(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 (:asurkov)]

(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)]

(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 (:asurkov)]

(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]]

(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]]

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]]

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 (:asurkov)]

(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] (NeedInfo me for attention)]

Removing in-testsuite request based on comment 24.