366438 - getTextAtOffset for line incorrect when whitespace precedes <br>

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[parente]

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.

Comment 1

[Scott Haeger]

Removing the <br /> element in the footer fixes the review hang/cycle problem.

Comment 2

[Aaron Leventhal]

You can keep the <br /> but if you remove the whitespace before it that also fixes the problem.

Comment 3

[Aaron Leventhal]

Attachment: Minimal testcase for problems

Comment 4

[parente]

Any new status on this bug?

Comment 5

[Aaron Leventhal]

I'm halfway through it but it got delayed. I'll work on it next week.

Comment 6

[parente]

Cool. Thanks.

Comment 7

[Aaron Leventhal]

Also, our last attempt to get <br> right was in bug 366438

See also: bug 352533.

Comment 8

[Aaron Leventhal]

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.

Comment 9

[Aaron Leventhal]

> 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 10

[Aaron Leventhal]

Attachment: 1) Use eSelectBeginLine followed by eSelectCharacter to get line break, 2) While here, fix problem on missing char on last line of para

Comment 11

[Uri Bernstein]

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

Comment 12

[Aaron Leventhal]

Attachment: Described in bug comment

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()

Comment 13

[Aaron Leventhal]

Attachment: Comments and more code simplification

Comment 14

[Aaron Leventhal]

Attachment: Comments and more code simplification

Comment 15

[alexander :surkov (:asurkov)]

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.

Comment 16

[Aaron Leventhal]

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.