Closed Bug 254997 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: Disability Access APIs, defect)

1.7 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: korn, Assigned: ginnchen+exoracle)

References

()

Details

(Keywords: access, Whiteboard: AP1)

Attachments

(3 files, 2 obsolete files)

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.
Whiteboard: AP1
Blocks: caretnav
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.
Assignee: aaronleventhal → ginn.chen
OS: Linux → All
Hardware: Sun → All
Yes Aaron, I'm aware they are working on this.
Attached file test case (obsolete) —
Simple test case
Attached patch patch (obsolete) — Splinter Review
Dig into table and table cell to find nsILineIteratorNavigator.
Comment on attachment 156313 [details] [diff] [review]
patch

review?
Attachment #156313 - Flags: superreview?(roc)
Attachment #156313 - Flags: review?(aaronleventhal)
*** Bug 241044 has been marked as a duplicate of this bug. ***
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;
Attachment #156313 - Flags: superreview?(roc)
Attachment #156313 - Flags: review?(aaronleventhal)
Attachment #156313 - Flags: review-
Attached file downarrow_testcase
Sorry, the old testcase is a wrong file, Sorry for my careless.
Attachment #156312 - Attachment is obsolete: true
Attached patch patch 1.1Splinter Review
the while(frame) will protect now. It was my last refining's error.
Attachment #156313 - Attachment is obsolete: true
Attachment #156475 - Flags: review?(aaronleventhal)
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.
Attachment #156475 - Flags: review?(aaronleventhal) → review+
(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
> 
Attachment #156757 - Flags: superreview?(roc)
Attachment #156757 - Flags: review+
Attachment #156757 - Flags: superreview?(roc) → superreview+
fixed.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.