Last Comment Bug 655264 - clean up some a11y code and make it less reliant on views
: clean up some a11y code and make it less reliant on views
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Timothy Nikkel (:tnikkel)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 655263
  Show dependency treegraph
 
Reported: 2011-05-06 08:41 PDT by Timothy Nikkel (:tnikkel)
Modified: 2011-05-31 07:31 PDT (History)
4 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.38 KB, patch)
2011-05-06 08:44 PDT, Timothy Nikkel (:tnikkel)
roc: review+
surkov.alexander: feedback+
Details | Diff | Splinter Review

Description Timothy Nikkel (:tnikkel) 2011-05-06 08:41:17 PDT

    
Comment 1 Timothy Nikkel (:tnikkel) 2011-05-06 08:44:10 PDT
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.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-08 22:33:48 PDT
Comment on attachment 530640 [details] [diff] [review]
patch

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

Nice!
Comment 3 Timothy Nikkel (:tnikkel) 2011-05-27 11:15:06 PDT
Alexander, did you want to have a look at this patch?
Comment 4 Marco Zehe (:MarcoZ) 2011-05-27 11:25:10 PDT
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) 2011-05-27 11:43:52 PDT
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 alexander :surkov 2011-05-29 22:10:21 PDT
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) 2011-05-30 10:23:36 PDT
(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) 2011-05-30 11:25:40 PDT
http://hg.mozilla.org/projects/cedar/rev/b55b6d578d4d
Comment 9 Mounir Lamouri (:mounir) 2011-05-31 07:31:23 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/b55b6d578d4d

Note You need to log in before you can comment on or make changes to this bug.