376275 - getTextAtOffset returns bad text, start, end offsets for bulletted list

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Scott Haeger]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a4pre) Gecko/20070401 Minefield/3.0a4pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a4pre) Gecko/20070401 Minefield/3.0a4pre

Something changed from the 20070320 build and the 20070321 build that broke getTextAtOffset for bulletted lists.

This bug may be related to https://bugzilla.mozilla.org/show_bug.cgi?id=373077
Test case in this bug can be used to demonstrate the problem.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[alexander :surkov (:asurkov)]

Are there instructions how to test or testcase?

Comment 2

[Aaron Leventhal]

To test nsIAccessibleText you can do 2 things:
1) Use Accerciser, but that's on Linux. You probably want to set that up eventually anyway, because much of this will need to be tested on Linux as well.
2) Test using Javascript -- http://www.mozilla.org/access/qa/jstest.html

Other than that, you will have to wait until something like June to get a copy of a test tool that you can use on a daily basis.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

I tried to get rid assertion from nsIContent->GetTextLength() but it looks the problem has been gone too ;)

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> Created an attachment (id=260992) [details]
> patch
> 
> I tried to get rid assertion from nsIContent->GetTextLength() but it looks the
> problem has been gone too ;)
> 

Ah, actually I don't see a problem on trunk. The patch get rid assertion only. For tests I used http://www.mozilla.org/access/today page.

Comment 5

[Aaron Leventhal]

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

Why do we deal with list bullets both in nsAccessible::GetContentText() and nsHTMLListBulletAccessible::GetContentText() ?

Also, should nsAccessible::GetContentText() just return an empty string, and then have more specific implementation in places like nsHTMLTextAccessible?


pAcc->GetContentText(newText);

The old code for nsHyperTextAccessible::GetPosAndText() is more performant, because it avoids extra string copies when newText is not needed. It just gets the length directly from the content's storage of that.
     if (IsText(accessible)) {
-      // Avoid string copies
-      PRInt32 substringEndOffset = frame->GetContent()->TextLength();
+      nsCOMPtr<nsPIAccessible> pAcc(do_QueryInterface(accessible));
       nsAutoString newText;
-      if (!substringEndOffset) {
-        // This is exception to the frame owns the text.
-        // The only known case where this occurs is for list bullets
-        // We could do this for all accessibles but it's not as performant
-        // as dealing with nsIContent directly
-        accessible->GetName(newText);
-        substringEndOffset = newText.Length();
-      }
+      pAcc->GetContentText(newText);
+
+      PRInt32 substringEndOffset = newText.Length();

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> (From update of attachment 260992 [details] [diff] [review])
> Why do we deal with list bullets both in nsAccessible::GetContentText() and
> nsHTMLListBulletAccessible::GetContentText() ?

We don't deal with list bullets in nsAccessible::GetContentText() actually. I just leaved this code to be common. If you don't care then I'll remove this.

> Also, should nsAccessible::GetContentText() just return an empty string, and
> then have more specific implementation in places like nsHTMLTextAccessible?

Agree. Probabaly nsTextAccessible?

> 
> pAcc->GetContentText(newText);
> 
> The old code for nsHyperTextAccessible::GetPosAndText() is more performant,
> because it avoids extra string copies when newText is not needed. It just gets
> the length directly from the content's storage of that.

Not sure I got you. Now we form newText from GetName() and from nsIContent::appendTextTo(). New code is the same like the old one when startOffset < substringEndOffset. How often this statement is false?

Btw, can you confirm or not the problem because I don't see it?

Comment 7

[Aaron Leventhal]

Yes, please remove the extra code in nsAccessible::GetContentText() and then move the whole thing to nsTextAccessible.

> New code is the same like the old one when
> startOffset < substringEndOffset. How often this statement is false?
I'm not sure. Let's say you are getting some text at the end of a paragraph with many images and links and text formats in it. For every new child, there will be a n extra string copy. Maybe it's not so bad, so I guess we can probably keep it.

> Btw, can you confirm or not the problem because I don't see it?
You mean the original bug report? This patch just fixes an assertion?

Comment 8

[Aaron Leventhal]

Scott, the testcase that was posted in the other bug is more complicated than just a list. It's a list with items that are links.

Is the bug with any list item or only with list items that are links?

Comment 9

[Scott Haeger]

Attachment: test case #1 (generic list)

The problem only exists for lists with links.  The attached test case is a list of text items and demonstrates proper function.  The previous test case shows the problem when links are present.

Comment 10

[Aaron Leventhal]

Comment on attachment 261025 [details]
test case #1 (generic list)


                      Plain list items     Linked list items
At offset 0                  ?                    ?
Offset after bullet        broken                 ?

The patch doesn't test everything here.

Surkov, you're going to have to mess around with that JS testcase to be able to look at everything.

Comment 11

[Scott Haeger]

Attachment: replaces test case #1

The old version of test case #1 has two markup errors.  This version fixes those errors but it should be noted that the old version (with errors) illustrates a problem seen a couple times, namely the generation of a bogus accessible.  I will create a new bug that is specific to this problem.

Comment 12

[Aaron Leventhal]

Surkov, you can use this page to test (once it updates in an hour):
http://www.mozilla.org/access/qa/linetest/jstest.html
If you click on an item or navigate to it somehow, it will show you the text and offsets at line 1, 2, 3, etc. for that item.

1. Click on the first (plain) list item, and see that the text starts at offset 3 instead of offset 0.
2. Click-drag on the second (linked) list item, and then hit Alt+Shift+P to go to the parent. Notice the accessible text for the line is blank.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> Surkov, you can use this page to test (once it updates in an hour):
> http://www.mozilla.org/access/qa/linetest/jstest.html
> If you click on an item or navigate to it somehow, it will show you the text
> and offsets at line 1, 2, 3, etc. for that item.
> 
> 1. Click on the first (plain) list item, and see that the text starts at offset
> 3 instead of offset 0.

I thought it's right behavior because this offset is added by list bullet. If it's not right then it's easy to fix if we'll stop to take in account list bullet.

> 2. Click-drag on the second (linked) list item, and then hit Alt+Shift+P to go
> to the parent. Notice the accessible text for the line is blank.
> 

Do you mean that parent accessible should expose text from child accessibles? In your example, TD that contains A expose it (Window-Eyes Screen Reader), LI doesn't do it (Linked List item) and H2 doesn't do it too (LSR - GNOME Live!). Which kind of accessible should expose child's text? How does it depend on child accessible types?

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch2

get rid assertion for list bullet

Comment 15

[Aaron Leventhal]

> I thought it's right behavior because this offset is added by list bullet. 

So, you're in the hypertext that includes the list bullet and the list item text. When you ask for the start of the link from any hyper text, you should get:
1) if it goes to the the start of the line, you should get the start of the line, (which in this case is at offset 0)
2) otherwise, if the hypertext doesn't go to or past the start of the line, it should return the start of the hypertext

> Do you mean that parent accessible should expose text from child accessibles?
I'm not looking at a TD -- we must be looking at 2 different things.
I'm looking at "Linked list item" which is:
<li><a href="http://www.google.com">Linked List item</a></li>

I just asked you to go to the parent because the mousedown for the click-frag occurs on the link. Both it and the list item are hypertexts.
- If you as for the start of the line for the link, you'll get position 0 for that, but it's scenario #2 from above because the link doesn't reach the start of the line.
- Go to the parent and get theline for the list item's hypertext. That should be starting at position 0 and return "2. Linked list item". Right now it returns nothing.

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 261104 [details] [diff] [review]
patch2

checked in

Comment 17

[Aaron Leventhal]

Surkov, you're leaving this open to address the remaining issues in comment 15, correct?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> Surkov, you're leaving this open to address the remaining issues in comment 15,
> correct?
> 

yes

Comment 20

[Aaron Leventhal]

Attachment: 1) Hack to set start offset to 0 in the bullet case, 2) fix DOMPointToOffset() when offset is at the very end of grandchild hypertext

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 262595 [details] [diff] [review]
1) Hack to set start offset to 0 in the bullet case, 2) fix DOMPointToOffset() when offset is at the very end of grandchild hypertext

r=me, I don't see problems