Added aria-hidden attribute to elements obscured behind an overlay to aid screen readers.

Added aria-hidden attribute to elements obscured behind an overlay to aid screen readers.

BUG=113337
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138003

[dmazzoni, 2012-05-08 22:48:51]

Thanks! I'll test this and make sure it produces the desired results.

I think we'll also need to hide the uber frame whenever an overlay is showing.

[Kyle Horimoto, 2012-05-08 23:17:24]

On 2012/05/08 22:48:51, Dominic Mazzoni wrote:
> Thanks! I'll test this and make sure it produces the desired results.
> 
> I think we'll also need to hide the uber frame whenever an overlay is showing.

Cool, I added that in on the other CL
(https://chromiumcodereview.appspot.com/10388034/diff/3001/chrome/browser/reso...).

[Evan Stade, 2012-05-09 22:37:37]

http://codereview.chromium.org/10382073/diff/1/chrome/browser/resources/optio...
File chrome/browser/resources/options2/options_page.js (right):

http://codereview.chromium.org/10382073/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/options_page.js:240: var topmostPage =
this.getTopmostVisiblePage();
seems like this might do the wrong thing in certain cases, such as if you called
showOverlay twice on the same overlay (which can probably happen). Or if you
call showOverlay while dialog A is showing, then call showOverlay on dialog B,
then showOverlay on A again. A will still have aria-hidden on true.

[Kyle Horimoto, 2012-05-10 22:18:45]

https://chromiumcodereview.appspot.com/10382073/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options2/options_page.js (right):

https://chromiumcodereview.appspot.com/10382073/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/options_page.js:240: var topmostPage =
this.getTopmostVisiblePage();
On 2012/05/09 22:37:37, Evan Stade wrote:
> seems like this might do the wrong thing in certain cases, such as if you
called
> showOverlay twice on the same overlay (which can probably happen). Or if you
> call showOverlay while dialog A is showing, then call showOverlay on dialog B,
> then showOverlay on A again. A will still have aria-hidden on true. 

Done.

[dmazzoni, 2012-05-10 23:11:25]

Just patched and tested this out. Looks like it's mostly working but you also
need to apply aria-hidden to the search box, it's outside of the container you
placed aria-hidden on.

I'll be out for two days, so once you've fixed the search box, feel free to
commit once you have the lgtm from Evan.

Plus, it will be easier for me to test when all of your changes have landed.
This change in isolation doesn't work as well without the focus fixes still in
progress.

But either way this is a great improvement, it's going to be a much nicer
experience for keyboard and screen reader users soon. Thanks!

[Kyle Horimoto, 2012-05-10 23:13:41]

On 2012/05/10 23:11:25, Dominic Mazzoni wrote:
> Just patched and tested this out. Looks like it's mostly working but you also
> need to apply aria-hidden to the search box, it's outside of the container you
> placed aria-hidden on.
> 
> I'll be out for two days, so once you've fixed the search box, feel free to
> commit once you have the lgtm from Evan.
> 
> Plus, it will be easier for me to test when all of your changes have landed.
> This change in isolation doesn't work as well without the focus fixes still in
> progress.
> 
> But either way this is a great improvement, it's going to be a much nicer
> experience for keyboard and screen reader users soon. Thanks!

Great, I'll incorporate the search box in. Thanks for your help Dominic!

[Kyle Horimoto, 2012-05-11 21:33:03]

On 2012/05/10 23:13:41, Kyle Horimoto wrote:
> On 2012/05/10 23:11:25, Dominic Mazzoni wrote:
> > Just patched and tested this out. Looks like it's mostly working but you
also
> > need to apply aria-hidden to the search box, it's outside of the container
you
> > placed aria-hidden on.
> > 
> > I'll be out for two days, so once you've fixed the search box, feel free to
> > commit once you have the lgtm from Evan.
> > 
> > Plus, it will be easier for me to test when all of your changes have landed.
> > This change in isolation doesn't work as well without the focus fixes still
in
> > progress.
> > 
> > But either way this is a great improvement, it's going to be a much nicer
> > experience for keyboard and screen reader users soon. Thanks!
> 
> Great, I'll incorporate the search box in. Thanks for your help Dominic!

Added code to apply aria-hidden to the search box. PTAL, thanks!

[Evan Stade, 2012-05-15 21:37:32]

http://codereview.chromium.org/10382073/diff/9001/chrome/browser/resources/op...
File chrome/browser/resources/options2/options_page.js (right):

http://codereview.chromium.org/10382073/diff/9001/chrome/browser/resources/op...
chrome/browser/resources/options2/options_page.js:113: this.removeAriaHidden_();
sprinkling this.removeAriaHidden_() all across this file doesn't seem like a
particularly robust way to solve the problem. Next time someone needs to make a
change, what are the chances they'll think about whether they need to make this
call? Also it's really hard to know that all possible situations are handled
correctly.

[dmazzoni, 2012-05-15 21:42:48]

On 2012/05/15 21:37:32, Evan Stade wrote:
>
http://codereview.chromium.org/10382073/diff/9001/chrome/browser/resources/op...
> File chrome/browser/resources/options2/options_page.js (right):
> 
>
http://codereview.chromium.org/10382073/diff/9001/chrome/browser/resources/op...
> chrome/browser/resources/options2/options_page.js:113:
this.removeAriaHidden_();
> sprinkling this.removeAriaHidden_() all across this file doesn't seem like a
> particularly robust way to solve the problem. Next time someone needs to make
a
> change, what are the chances they'll think about whether they need to make
this
> call? Also it's really hard to know that all possible situations are handled
> correctly.

I was thinking the same thing.

Could you integrate it with willHidePage and willShowPage? Maybe notifying a
page that it's hidden (or obscured?) should make it mark itself as aria-hidden.

[Kyle Horimoto, 2012-05-16 23:39:19]

On 2012/05/15 21:42:48, Dominic Mazzoni wrote:
> On 2012/05/15 21:37:32, Evan Stade wrote:
> >
>
http://codereview.chromium.org/10382073/diff/9001/chrome/browser/resources/op...
> > File chrome/browser/resources/options2/options_page.js (right):
> > 
> >
>
http://codereview.chromium.org/10382073/diff/9001/chrome/browser/resources/op...
> > chrome/browser/resources/options2/options_page.js:113:
> this.removeAriaHidden_();
> > sprinkling this.removeAriaHidden_() all across this file doesn't seem like a
> > particularly robust way to solve the problem. Next time someone needs to
make
> a
> > change, what are the chances they'll think about whether they need to make
> this
> > call? Also it's really hard to know that all possible situations are handled
> > correctly.
> 
> I was thinking the same thing.
> 
> Could you integrate it with willHidePage and willShowPage? Maybe notifying a
> page that it's hidden (or obscured?) should make it mark itself as
aria-hidden.

I moved some of the calls around a bit to make it slightly less confusing, but
I'm having a difficult time figuring out how to do this in a more elegant way.
Think you could provide some suggestions?

* Integrating it with didClosePage/didShowPage/handleCancel is difficult because
not every page has defined these functions. In addition, even if they did, it
would be difficult to coordinate because often to show one page we must also
show many pages (e.g., if A is B's parent and B is C's parent, showPage(C) will
call showPage(B) which will call showPage(A), so each of the didShowPage
handlers will be called, not only C's).

* It is necessary to hide the aria-hidden attribute both after a page is closed
and after it is canceled. This is because after a close/cancel, a new page is
the topmost page, and thus it cannot have the aria-hidden attribute anymore.

* It is also necessary to hide the aria-hidden attribute after showing a page
because of the possibility of calling showOverlay() in unusual ways (e.g.,
calling showOverlay() twice on the same overlay or calling showOverlay(A),
showOverlay(B), showOverlay(A)). In these cases, not removing the aria-hidden
attribute could cause it to still be there when it should not be.

Thus, I currently call it in four places
1) After a cancelOverlay,
2) After a closeOverlay,
3) After a showPage that results in a root page, and
4) After a showPage that results in an overlay

Any ideas of what a more elegant solution could look like? Thanks!

[Evan Stade, 2012-05-17 01:14:42]

everything goes through setOverlayVisible_. Let's take advantage of that. When
|visible| is true, set aria-hidden to false. When there is a parent page, set
its aria-hidden to true. When |visible| is false, you can leave aria-hidden
alone (since |this| will be true-hidden anyway). You do have to mark the parent
page to aria-hidden false. Any flaws you can see with this plan?

[Kyle Horimoto, 2012-05-17 07:09:26]

On 2012/05/17 01:14:42, Evan Stade wrote:
> everything goes through setOverlayVisible_. Let's take advantage of that. When
> |visible| is true, set aria-hidden to false. When there is a parent page, set
> its aria-hidden to true. When |visible| is false, you can leave aria-hidden
> alone (since |this| will be true-hidden anyway). You do have to mark the
parent
> page to aria-hidden false. Any flaws you can see with this plan?

Sounds like a good plan - I'll go ahead and do that tomorrow.

[Kyle Horimoto, 2012-05-17 21:16:16]

Does this look better?

[dmazzoni, 2012-05-17 21:30:14]

lgtm

Looks much cleaner and simpler to me!

[Evan Stade, 2012-05-18 23:15:16]

lgtm if it works

[Evan Stade, 2012-05-18 23:16:38]

On 2012/05/18 23:15:16, Evan Stade wrote:
> lgtm if it works

(I should always use this disclaimer!)

[commit-bot: I haz the power, 2012-05-18 23:39:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10382073/16002

[commit-bot: I haz the power, 2012-05-19 01:21:33]

Change committed as 138003