254997 - Caret tracking by line (down arrow) skips over huge portions of the document

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Peter Korn]

1. Launch Mozilla with accessibility support
2. Set your home page to http://www.sun.com
3. Press F7 to turn on caret browsing mode
4. Launch Gnopernicus with speech on
5. Exit Mozilla and then re-start it 
6. Hear the words: "Switch to Window Sun Microsystems html container".  Note
that the blinking caret is not presently visible on the screen
7. Press TAB.  This will bring the caret to the start of the document (note: you
hear no speech at this point)
8. Press down-arrow.  Hear "sun com"
9. Press down-arrow again, hear nothing.  Again, and nothing. [BUGS]
10. Press down-arrow a third time, hear "Low Cost Computing" and see the caret
at the end of that line.  Mozilla just skipped over most of that colum of the
web page! [BUG]
11. Press down-arrow again, hear nothing.  Again, and nothing. [BUGS]
12. Press down-arrow the sixth time from the start, and hear "Sun Microsystems
2004", with the caret again at the end of that text object.  Repeated presses
result in no speech.  How to distinguish this situation from the silence of before?
13. Now start pressing up arrow.  When you get to "Sun Microsystem 2004", the
caret appears there, but you get no speech (no event fired?) [BUG]
14. Up arrow again results in "Low Cost Computing"
15. Up arrow again "N1 Grid", again to "Sun and AMD Opteron", etc. all works
until you reach "sun com" again.  
16. Two more up-arrows and you wrap around to the bottom!  [BUG]

Note: this bug, or much of it, is probably a dup...  Filing again to be sure,
and as part of an enumeration of a bunch of UNIX mozilla accessibility bugs
recently found in a walk-through.

Comment 1

[Aaron Leventhal]

Peter, you realize Sun China is working on this stuff right? You might want to
email your bugs to them and let them check for dups and file them so we can
avoid duplication of effort.

Ginn Chen is working on this stuff.

Comment 2

[Peter Korn]

Yes Aaron, I'm aware they are working on this.

Comment 3

[Ginn Chen]

Attachment: test case

Simple test case

Comment 4

[Ginn Chen]

Attachment: patch

Dig into table and table cell to find nsILineIteratorNavigator.

Comment 5

[Ginn Chen]

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

review?

Comment 6

[Ginn Chen]

*** Bug 241044 has been marked as a duplicate of this bug. ***

Comment 7

[Aaron Leventhal]

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

>             PRBool searchTableBool = PR_FALSE;
>-            if (aPos->mResultFrame->GetType() == nsLayoutAtoms::tableOuterFrame)
>+            if (aPos->mResultFrame->GetType() == nsLayoutAtoms::tableOuterFrame
>+              || aPos->mResultFrame->GetType() == nsLayoutAtoms::tableCellFrame)
Fix whitespace to use Moz coding style like this:
>+            if (aPos->mResultFrame->GetType() == nsLayoutAtoms::tableOuterFrame ||
>+                aPos->mResultFrame->GetType() == nsLayoutAtoms::tableCellFrame)

>             {
>-              nsIFrame *frame = aPos->mResultFrame->GetFirstChild(nsnull);
>-              result = NS_OK;
>+              nsIFrame *frame = aPos->mResultFrame;
>               //got the table frame now
>               while(frame) //ok time to drill down to find iterator
>               {
>                 frame = frame->GetFirstChild(nsnull);
>-                if (frame && NS_SUCCEEDED(result))
>+                result = frame->QueryInterface(NS_GET_IID(nsILineIteratorNavigator),
>+                                                          getter_AddRefs(iter));
Why is null check no longer necessary? Looks dangerous. We usually skip null
checks on do_QueryInterface() because that takes care of the null check.
However, this calling a method on a potentially null pointer, right?

>+                if (NS_SUCCEEDED(result))
>                 {
>-                  result = frame->QueryInterface(NS_GET_IID(nsILineIteratorNavigator),
>-                                                            getter_AddRefs(iter));
>-                  if (NS_SUCCEEDED(result))
>-                  {
>-                    aPos->mResultFrame = frame;
>-                    searchTableBool = PR_TRUE;
>-                    break; //while aPos->mResultFrame
>-                  }
>+                  aPos->mResultFrame = frame;
>+                  searchTableBool = PR_TRUE;
>+                  break; //while aPos->mResultFrame
This comment should say // while (frame)

>                 }
>-                else
>-                  break;
>               }
>             }
>             if (!searchTableBool)
>               result = aPos->mResultFrame->QueryInterface(NS_GET_IID(nsILineIteratorNavigator),
>                                                         getter_AddRefs(iter));
>             if (NS_SUCCEEDED(result) && iter)//we've struck another block element!
>             {
>               doneLooping = PR_FALSE;

Comment 8

[Ginn Chen]

Attachment: downarrow_testcase

Sorry, the old testcase is a wrong file, Sorry for my careless.

Comment 9

[Ginn Chen]

Attachment: patch 1.1

the while(frame) will protect now. It was my last refining's error.

Comment 10

[Aaron Leventhal]

Comment on attachment 156475 [details] [diff] [review]
patch 1.1

1. Please check how this interacts with Composer
2. Please fix the whitespace in the if (I mentioned this in the last comment).
No big deal, this file already has bad whitespace, but reviewers want you to
get used to the current Mozilla coding style.
3. Check closer to see if there is a case where 
|result| is uninitialized if the while loop never gets entered.

Comment 11

[Ginn Chen]

(In reply to comment #10)
> (From update of attachment 156475 [details] [diff] [review])
> 1. Please check how this interacts with Composer
Same bug, same resolution
> 2. Please fix the whitespace in the if (I mentioned this in the last comment).
> No big deal, this file already has bad whitespace, but reviewers want you to
> get used to the current Mozilla coding style.
I missed this, I'll attach a new patch soon.
> 3. Check closer to see if there is a case where 
> |result| is uninitialized if the while loop never gets entered.
I believe it's no use to initialize it.
It while loop never gets entered, it will enter "if (!searchTableBool)" below
> 

Comment 12

[Ginn Chen]

Attachment: patch (fix coding style)

Comment 13

[Kyle Yuan]

fixed.