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

RESOLVED FIXED in mozilla7

Status

()

Core
Layout: View Rendering
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

Trunk
mozilla7
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 530640 [details] [diff] [review]
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.
Attachment #530640 - Flags: review?(roc)
Attachment #530640 - Flags: feedback?(surkov.alexander)
Comment on attachment 530640 [details] [diff] [review]
patch

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

Nice!
Attachment #530640 - Flags: review?(roc) → review+
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

6 years ago
Hi Marco. You can get a try build here http://stage.mozilla.org/pub/mozilla.org/firefox/try-builds/old/tnikkel@gmail.com-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

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

Comment 7

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

Comment 8

6 years ago
http://hg.mozilla.org/projects/cedar/rev/b55b6d578d4d
Whiteboard: [fixed-in-cedar][needs landing]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/b55b6d578d4d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar][needs landing]
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.