Some chrome accessibles report incorrect visibility states

VERIFIED FIXED in mozilla12

Status

()

Core
Disability Access APIs
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Jamie, Assigned: davidb)

Tracking

({regression})

Trunk
mozilla12
x86_64
Windows 7
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
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

6 years ago
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.
Quite possibly that bug yeah, and would be good to know for sure.

Comment 4

6 years ago
Regression range: January 10, 2012 is OK, January 11, 2012 is broken. Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c713003d3226&tochange=e79ef0ffcb09
Depends on: 591363
Assignee: nobody → bolterbugz
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.
Attachment #592765 - Flags: review?(marco.zehe)
Attachment #592765 - Flags: feedback?(surkov.alexander)
Attachment #592765 - Flags: review?(trev.saunders)
Created attachment 592774 [details] [diff] [review]
patch
Attachment #592765 - Attachment is obsolete: true
Attachment #592765 - Flags: review?(trev.saunders)
Attachment #592765 - Flags: review?(marco.zehe)
Attachment #592765 - Flags: feedback?(surkov.alexander)
Attachment #592774 - Flags: review?(trev.saunders)
Attachment #592774 - Flags: review?(marco.zehe)
Created attachment 592776 [details] [diff] [review]
patch (qrefreshed this time)
Attachment #592774 - Attachment is obsolete: true
Attachment #592774 - Flags: review?(trev.saunders)
Attachment #592774 - Flags: review?(marco.zehe)
Attachment #592776 - Flags: review?(marco.zehe)
We need to get these cases under test. Setting flag so we don't forget.
Flags: in-testsuite?
Created attachment 592790 [details] [diff] [review]
Moves ancestor visibility check to end (like we used to have).
Attachment #592776 - Attachment is obsolete: true
Attachment #592776 - Flags: review?(marco.zehe)
Attachment #592790 - Flags: review?(marco.zehe)
Pushed to try, included a11y talos: https://hg.mozilla.org/try/rev/ea0abaa7ca3f
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.
Attachment #592790 - Flags: review?(marco.zehe) → review+
Attachment #592790 - Flags: review?(surkov.alexander)
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.
I only did debug builds (silly me):
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbolter@mozilla.com-ea0abaa7ca3f/

Comment 14

6 years ago
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

6 years ago
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+
Attachment #592790 - Flags: review?(surkov.alexander) → feedback+
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?
(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

6 years ago
(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
(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

6 years ago
(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
(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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Reporter)

Comment 22

6 years ago
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
Status: RESOLVED → VERIFIED
(Reporter)

Comment 23

6 years ago
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

6 years ago
(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.
Removing in-testsuite request based on comment 24.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.