Closed Bug 366438 Opened 15 years ago Closed 15 years ago

getTextAtOffset for line incorrect when whitespace precedes <br>

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: parente, Assigned: aaronlev)

References

(Blocks 1 open bug, )

Details

(Keywords: access)

Attachments

(2 files, 3 obsolete files)

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061218 Minefield/3.0a1

In the foot of the page at http://www.freestandards.org/en/Accessibility there is a legal paragraph that reads:

Copyright © 2006 Free Standards Group. All rights reserved.
LSB is a trademark of the Free Standards Group. Linux is a registered trademark of Linus Torvalds.

Querying the accessible for this paragraph using:

text.getTextAtOffset(0, TEXT_BOUNDARY_LINE_START)

yields the following values:

line = ""
start offset = 0
end offset = 0

The expected value would be something like:

"Copyright © 2006 Free Standards Group. All rights reserved."
start offset = 0
end offset = 59

The getTextAtOffset method works on all other paragraphs, list items, links, etc. on the page. There's something about this paragraph that causes it to fail. This is problematic for a screen reader user who is trying to navigate the page by elements and lines.

Note that if text.getText(0, -1) is called to fetch all text in the accessible, it is returned properly.
Removing the <br /> element in the footer fixes the review hang/cycle problem.
You can keep the <br /> but if you remove the whitespace before it that also fixes the problem.
Summary: getTextAtOffset returns incorrect value → getTextAtOffset for line incorrect when whitespace precedes <br>
Any new status on this bug?
I'm halfway through it but it got delayed. I'll work on it next week.
Cool. Thanks.
Also, our last attempt to get <br> right was in bug 366438

See also: bug 352533.
Blocks: texta11y
Status: NEW → ASSIGNED
Keywords: access
OS: Linux → All
Hardware: PC → All
Sorry, the last attempt was bug 357625.

What is the best way to get the end of the line? Using eSelectEndLine with nsPeekOffsetStruct is not getting us to the <br> when there are spaces before it.
> What is the best way to get the end of the line? Using eSelectEndLine with
> nsPeekOffsetStruct is not getting us to the <br> when there are spaces before
> it.
That part is actually working.
The problem is our code to get the start of the line is using eSelectLine, and is going back to the previous line, before the <br> and spaces before that.
Comment on attachment 254208 [details] [diff] [review]
1) Use eSelectBeginLine followed by eSelectCharacter to get line break, 2) While here, fix problem on missing char on last line of para

>+  if (NS_SUCCEEDED(rv) && aAmount == eSelectBeginLine) {
>+    // Start of line fixup: include \n or <br> in start of line
>+    // This will also skip past any unwanted span level markup at the start of line,
>+    // so we'll truly be on the line break
>+    pos.SetData(eSelectCharacter, eDirPrevious, pos.mContentOffset, 0, kIsJumpLinesOk,
>+                kIsScrollViewAStop, kIsKeyboardSelect, kIsVisualBidi,
>+                wordMovementType);    
>+    rv = aFromFrame->PeekOffset(&pos);
>+  }


You can't call the second PeekOffset() on aFromFrame, because that's not necessarily the frame you're on after moving to the beginning of the line.
Unfortunately, according to the comment in [1], you can't use pos.mResultFrame either. So you'll have to call GetFrameForNodeOffset(), like that code does.

Other than that, I assume you tested this (at least for some common cases) and it works for you. I don't really have the means to test it.

[1] http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsSelection.cpp&rev=3.277&mark=1358-1359#1357
Attached patch Described in bug comment (obsolete) — Splinter Review
This fix does not require improving the results we get from layout. We make a special case for <br>, and the last line of a hypertextaccessible and that's it.

I've tested this new code with as many testcases as I can think of from here:
http://www.mozilla.org/access/qa/linetest/jstest.html

1) Move code which walks the DOM tree to get the next available accessible. This used to be in nsDocAccessibleWrap::GetFirstLeafAccessible(). It's been moved to nsAccessible::GetNextAvailableAccessible().
2) Use GetNextAvailableAccessible() in part of nsHyperTextAccessible::DOMPointToOffset() that looks for the next sensible accessible to search for. Previously we just walked the children of the hypertext, the new method uses a depth first search of nodes.
3) Refactor DOMPointToOffset() so it uses the code described in #2 even when we're in a text node, because text nodes can be empty and have no accessible.
4) Pass back aIsAtEnd in DOMPointToOffset() and GetRelativeOffset(), since it's already calculated and can help decide what to do when getting a line at the end of a hypertextaccessible.
5) Fix GetRelativeOffset() so that it always uses the line break character for both the beginning and end of a line
6) Simplify GetTextHelper()
Attachment #254208 - Attachment is obsolete: true
Attachment #256566 - Flags: review?(surkov.alexander)
Attachment #254208 - Flags: review?(uriber)
Attachment #256566 - Attachment is obsolete: true
Attachment #256879 - Flags: review?(surkov.alexander)
Attachment #256566 - Flags: review?(surkov.alexander)
Attachment #256879 - Attachment is obsolete: true
Attachment #256882 - Flags: review?(surkov.alexander)
Attachment #256879 - Flags: review?(surkov.alexander)
Comment on attachment 256882 [details] [diff] [review]
 Comments and more code simplification  

I can't see problems with the patch. I hope I missed nothing. I have only nit:

>+  /*
>+   * This does the work for nsIAccessibleText::GetText[At|Before|After]Offset
>+   * @param aType, eGetBefore, eGetAt, eGetAfter
>+   * @param aBoundaryType, char/word-start/word-end/line-start/line-end/paragraph/attribute
>+   * @param aOffset, offset into the hypertext to start from
>+   * @param *aStartOffset, the resulting start offset for the returned substring
>+   * @param *aStartOffset, the resulting start offset for the returned substring

there are defined two @param aStartOffset.
Attachment #256882 - Flags: review?(surkov.alexander) → review+
Some regressions: bug 373036 and an issue with list items returning 0 for the start and end index.

Will deal with those in the separate bugs.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: lsr
Depends on: 506389
You need to log in before you can comment on or make changes to this bug.