655264 - clean up some a11y code and make it less reliant on views

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

We can simplify this code and make it less reliant on views (win-win!). I also added a check for activeness of the document which will be needed when views are removed from decks, because the hidden deck view will no longer be around.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 530640 [details] [diff] [review]
patch

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

Nice!

Comment 3

[Timothy Nikkel (:tnikkel)]

Alexander, did you want to have a look at this patch?

Comment 4

[Marco Zehe (:MarcoZ)]

Timothy, thanks for providing this! Did you throw it against a try-server and can give the build link? I'd like to give this a run through with a screen reader, and also the try-run will give us test results.

Comment 5

[Timothy Nikkel (:tnikkel)]

Hi Marco. You can get a try build here http://stage.mozilla.org/pub/mozilla.org/firefox/try-builds/old/[e-mail]-f96da2814f82/ but it might not be there much longer as I pushed to try a while ago now. That build also has some other changes besides the patch from this bug that I hope to land at some point. The patch in this bug was green on try server.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 530640 [details] [diff] [review]
patch

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

It's not evident for me that logic of old and new versions is preserved (nested document -> views loops vs views loop + presshell loop), I trust Roc on this way, f=me

::: accessible/src/base/nsAccessible.cpp
@@ +3245,1 @@
>  {

nit: please put return type into separate line

@@ +3245,5 @@
>  {
> +  nsIView* view = aFrame->GetClosestView();
> +  if (view && !view->IsEffectivelyVisible()) {
> +    return PR_FALSE;
> +  }

nit: please do no use braces around single if

::: accessible/src/base/nsAccessible.h
@@ +602,5 @@
>    //////////////////////////////////////////////////////////////////////////////
>    // Helpers
>  
>    // Check the visibility across both parent content and chrome
> +  PRBool CheckVisibilityInParentChain(nsIFrame* aFrame);

nit: please change PRBool to bool while you are here

nit: this is pure util method, it makes sense to move it to nsCoreUtils

::: accessible/src/base/nsDocAccessible.cpp
@@ +313,5 @@
>    }
>   
>    nsIFrame* frame = GetFrame();
> +   if (frame == nsnull ||
> +      !CheckVisibilityInParentChain(frame)) {

nit: please change to
if (!frame || !CheckVisibilityInParentChain())

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to comment #6)
> nit: please put return type into separate line

Done

> nit: please do no use braces around single if

Done.

> nit: please change PRBool to bool while you are here

Done.

> nit: this is pure util method, it makes sense to move it to nsCoreUtils

Done.

> nit: please change to
> if (!frame || !CheckVisibilityInParentChain())

Done.

Thanks.

Comment 8

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/projects/cedar/rev/b55b6d578d4d

Comment 9

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/b55b6d578d4d