1478964 - at-spi caret offset wrong when caret is set in bullet

Status: NEW

(Core :: Disability Access APIs, defect, P3)

Description

[Samuel Thibault]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180718215440

Steps to reproduce:

- Open data:text/html;charset=utf-8,<ul><li>foo</li></ul>
- Open accerciser, look for the object corresponding to the "foo" line
- In the Event Monitor, enable object:text-caret-moved
- In the python console, type acc.queryText().setCaretOffset(2)
- An object:text-caret-moved  event shows up in the Event Monitor with (2,0,0) as expected
- In the python console, type acc.queryText().setCaretOffset(1)   (or resp. (0))


Actual results:

On the second setCaretOffset call, an object:text-caret-moved event shows up in the Event Monitor with (5,0,0), i.e. as if the caret moved to the end of the line


Expected results:

The object:text-caret-moved event should have (1,0,0)   (or resp. (0,0,0))
Or it could have (2,0,0) since visually the caret is still right on the left of f. But definitely not (5,0,0).


I guess 5 comes from the fact that once acc.queryText().setCaretOffset(1) is done, acc.queryText().get_caretOffset() returns -1, which probably some part of firefox spuriously translates into "end of line".

Note: with firefox 61 setCaretOffset(1) behaves correctly, but setCaretOffset(0) still has the trouble.

Comment 1

[Samuel Thibault]

firefox 63 is also affected.

Comment 2

[Samuel Thibault]

I could identify the issue being in SelectionManager::ProcessTextSelChangeEvent : 

    mCaretOffset =  caretCntr->DOMPointToOffset(selection->GetFocusNode(),
                                                 selection->FocusOffset());

selection->FocusOffset() is 0, and caretCntr->DOMPointToOffset returns 5.

Comment 3

[Samuel Thibault]

Attachment: patch

I have tracked it down to HyperTextAccessible::TransformOffset which does this when it does not find a child to provide the offset:

   // If the given a11y point cannot be mapped into offset relative this hypertext
   // offset then return length as fallback value.
   return CharacterCount();

I would say that when aOffset was zero, a better fallback value would be 0, which happens to fix the issue, here is a proposed patch, being tested on

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a190f894173d4645b03353de958410cea3db368

Of course, the real issue is that the child found by HyperTextAccessible::DOMPointToOffset's call to GetchildAt_Deprecated (which I guess represents the bullet character) does not seem to have "this" as parent, which makes HyperTextAccessible::TransformOffset use the fallback value. I don't know how to easily investigate that though. At least the patch I propose here makes sense and should make the whole a11y stack stronger against such issues.

Comment 4

[Samuel Thibault]

It seems some testsuite results depend on the "length" fallback, I'll have a look.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Samuel Thibault from comment #3)

> Of course, the real issue is that the child found by
> HyperTextAccessible::DOMPointToOffset's call to GetchildAt_Deprecated (which
> I guess represents the bullet character) does not seem to have "this" as
> parent, which makes HyperTextAccessible::TransformOffset use the fallback
> value. I don't know how to easily investigate that though.

Right, it'd be interesting to understand what happens here, are there any mutation event happening around caret offset change events? I'm curious if we are in the middle of tree update process, when text offsets are queried, and thus we might need to adjust ordering.

> At least the
> patch I propose here makes sense and should make the whole a11y stack
> stronger against such issues.

It indeed looks like a hack. It'd be better to figure out what the real problem is, otherwise it may pop up at different places.

Also I'd say it makes sense to assert if we reach the fallback value, since it shouldn't happen in the wilds. And could you think of putting a test case?

Comment 6

[Samuel Thibault]

Attachment: patch

> are there any mutation event happening around caret offset change events?

No, the NotificationController::ProcessMutationEvents call that goes along the willRefresh does not find any mutation event from mFirstMutationEvent.

> It'd be better to figure out what the real problem is, otherwise it may pop up at different places.

Right. Looking more at the Accessible tree, it seems that in this case HyperTextAccessible::TransformOffset is called with aDescendant being "this" (the list item) itself. So the loop was deemed not to find it as a child of "this" :)  I tried to return 0 in that case but we end up with some of the same kind of testsuite issues as the other patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=872025caa54317fdc9c376452068f73f7d72eb5d&selectedJob=191320679 , so it seems that "this" is passed also in situations where the expected result really is the end of the HyperTextAccessible.

> Also I'd say it makes sense to assert if we reach the fallback value, since it shouldn't happen in the wilds.

Well, the failures with my first patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a190f894173d4645b03353de958410cea3db368 show that the fallback case does happen in the wild ATM :) Putting an assert brings even more failures, see https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aaccf3b730f12d68f5c4df7a7e823884d9dafd2&selectedJob=191314144 . I guess that when the caret is at end of some set of elements, it's really not easy for the accessibility stack to get right anyway since there is nothing at that offset (thus "Case #3" in HyperTextAccessible::DOMPointToOffset), and the fallback helps getting that case right: we are simply at the end. Perhaps HyperTextAccessible::TransformOffset just needs to be aware whether we are in Case #2 (falling back to beginning of the HyperTextAccessible) or Case #3 (falling back to end of the HyperTextAccessible). I have attached a patch doing so, testsuite is running on https://treeherder.mozilla.org/#/jobs?repo=try&revision=87695ec3f05318ee9fb5693d78b7a2ef1f2af337 , it seems to be going fine, so I guess the failures with my previous patch were all due to case #3.

> And could you think of putting a test case?

Sure, I'll have a look.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Samuel Thibault from comment #6)

> > are there any mutation event happening around caret offset change events?
> 
> No, the NotificationController::ProcessMutationEvents call that goes along
> the willRefresh does not find any mutation event from mFirstMutationEvent.

ok, good, so no mutations involved :)

> > It'd be better to figure out what the real problem is, otherwise it may pop up at different places.
> 
> Right. Looking more at the Accessible tree, it seems that in this case
> HyperTextAccessible::TransformOffset is called with aDescendant being "this"
> (the list item) itself. So the loop was deemed not to find it as a child of
> "this" :)  I tried to return 0 in that case but we end up with some of the
> same kind of testsuite issues as the other patch:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=872025caa54317fdc9c376452068f73f7d72eb5d&selectedJob=1
> 91320679 , so it seems that "this" is passed also in situations where the
> expected result really is the end of the HyperTextAccessible.

in case of |this|, I'd say the return value should be aOffset, also it could be 1) if aDescendant == this && aOffset == 0, then return 0, if aDescendant == this && aOffset == 1, then return CharacterCount().

> 
> > Also I'd say it makes sense to assert if we reach the fallback value, since it shouldn't happen in the wilds.
> 
> Well, the failures with my first patch
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3a190f894173d4645b03353de958410cea3db368 show that
> the fallback case does happen in the wild ATM :) Putting an assert brings
> even more failures, see
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0aaccf3b730f12d68f5c4df7a7e823884d9dafd2&selectedJob=1
> 91314144 . I guess that when the caret is at end of some set of elements,
> it's really not easy for the accessibility stack to get right anyway since
> there is nothing at that offset (thus "Case #3" in
> HyperTextAccessible::DOMPointToOffset), and the fallback helps getting that
> case right: we are simply at the end. Perhaps
> HyperTextAccessible::TransformOffset just needs to be aware whether we are
> in Case #2 (falling back to beginning of the HyperTextAccessible) or Case #3
> (falling back to end of the HyperTextAccessible). I have attached a patch
> doing so, testsuite is running on
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=87695ec3f05318ee9fb5693d78b7a2ef1f2af337 , it seems
> to be going fine, so I guess the failures with my previous patch were all
> due to case #3.

It is complicated. Does it fail in case of 'return 0 or CharacterCount()' approach?

Comment 8

[Samuel Thibault]

> if aDescendant == this && aOffset == 0, then return 0

I had tried that as well, but that gets the same issues: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8540e8f9b44680a0dce9917c5e2da6820bb8690c

> Does it fail in case of 'return 0 or CharacterCount()' approach?

So apparently yes.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Samuel Thibault from comment #8)
> > if aDescendant == this && aOffset == 0, then return 0
> 
> I had tried that as well, but that gets the same issues:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8540e8f9b44680a0dce9917c5e2da6820bb8690c
> 
> > Does it fail in case of 'return 0 or CharacterCount()' approach?
> 
> So apparently yes.

It feels hacky. I would argue that TransformOffset should translate any a11y point (i.e. an accessible and offset within the accessible) into a hypertext offset relative the root. Thus it's not clear what |fallbackAfter| propose is. Theoretically we should never reach fallback if given arguments are correct, and it if happens, then there's something wrong with the caller. Thus I'd say the real problem hides somewhere inside HyperTextAccessible::DOMPointToOffset. Sounds about right?

Comment 10

[Samuel Thibault]

> Theoretically we should never reach fallback if given arguments are correct, and it if happens, then there's something wrong with the caller. Thus I'd say the real problem hides somewhere inside HyperTextAccessible::DOMPointToOffset. Sounds about right?

Thinking about this, I checked in more details what happens in the cases failing in my tries. I happens that in these cases where aDescendant is |this|, aIsEndOffset is also set to true, so we should be returning CharacterCount() in that case. The caller can indeed not give anything better than the HyperTextAccessible itself, since we are at the end. I'll be trying that and seeing if that avoids having to rely on the fallback.

Comment 11

[Samuel Thibault]

Attachment: patch

Here is what I have been testing. This indeed avoids relying on the fallback in the cases where it's actually |this| which is passed as aDescendant. A few things, however:

- This does not avoid the fallback completely. Actually it can be argued that the fallback is the documented behavior: “Will return an offset for the end of the string if the node is not found.” I have noticed quite a few cases where this happens, the Accessible being passed is actually not a child of |this|, for instance when “if that node isn't accessible, use the accessible for the next DOM node which has one (based on forward depth first search)”, which makes sense: as I said, when it's the end of |this| which is to be returned, there is no child to give to TransformOffset.

- I added a test, but the "offset 1" case does not pass, I don't understand why: there really is a caret move event which is emitted, but apparently the checker somehow does not manage to see it.

- mochi tests pass, except on MacOS :   https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb4ed2e33810a67e1adcd82beb0d36551c49ab32
 , where there are:

    accessible/tests/mochitest/events/test_caretmove.html | Wrong caret offset for ['textarea@id="textarea" node', address: [object HTMLTextAreaElement], role: entry, address: [xpconnect wrapped nsIAccessible]] - got +0, expected 12

I don't understand why only that test would fail, and only on MacOS.

    accessible/tests/mochitest/events/test_focus_general.html | Can't get accessible for [ 'document node', address: [object HTMLDocument] ]

For which I don't understand the relation with my change, and why this would happen on MacOS only.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Samuel Thibault from comment #11)

sorry for delay in review

> Here is what I have been testing. This indeed avoids relying on the fallback
> in the cases where it's actually |this| which is passed as aDescendant. A
> few things, however:
> 
> - This does not avoid the fallback completely. Actually it can be argued
> that the fallback is the documented behavior: “Will return an offset for the
> end of the string if the node is not found.” I have noticed quite a few
> cases where this happens, the Accessible being passed is actually not a
> child of |this|, for instance when “if that node isn't accessible, use the
> accessible for the next DOM node which has one (based on forward depth first
> search)”, which makes sense: as I said, when it's the end of |this| which is
> to be returned, there is no child to give to TransformOffset.

it makes sense

> 
> - I added a test, but the "offset 1" case does not pass, I don't understand
> why: there really is a caret move event which is emitted, but apparently the
> checker somehow does not manage to see it.

this is about commented out lines? It indeed strange, not sure how 1 offset could be different from 0. I guess you can live them as is, but please add corresponding todos.

> - mochi tests pass, except on MacOS :  
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=bb4ed2e33810a67e1adcd82beb0d36551c49ab32
>  , where there are:
> 
>     accessible/tests/mochitest/events/test_caretmove.html | Wrong caret
> offset for ['textarea@id="textarea" node', address: [object
> HTMLTextAreaElement], role: entry, address: [xpconnect wrapped
> nsIAccessible]] - got +0, expected 12
> 
> I don't understand why only that test would fail, and only on MacOS.

Hard to tell without debugging, does the existing test fail or this is a new one? If the new test fail, then you can wrap it by is Mac check and mark as todo as well.

Comment 13

[Samuel Thibault]

>> apparently the checker somehow does not manage to see it.
> 
> this is about commented out lines? It indeed strange, not sure how 1 offset could be different from 0. I guess you can live them as is, but please add corresponding todos.

Actually I understood, the problem is that the event is sent to another object, because of another issue somewhere else in the code. In my latest work I have fixed it. That is however still getting issues on macos

> Hard to tell without debugging, does the existing test fail or this is a new one? If the new test fail, then you can wrap it by is Mac check and mark as todo as well.

It's an existing test which now fails.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Samuel Thibault from comment #13)
> >> apparently the checker somehow does not manage to see it.
> > 
> > this is about commented out lines? It indeed strange, not sure how 1 offset could be different from 0. I guess you can live them as is, but please add corresponding todos.
> 
> Actually I understood, the problem is that the event is sent to another
> object, because of another issue somewhere else in the code. In my latest
> work I have fixed it. That is however still getting issues on macos
> 
> > Hard to tell without debugging, does the existing test fail or this is a new one? If the new test fail, then you can wrap it by is Mac check and mark as todo as well.
> 
> It's an existing test which now fails.

then it's bad. according to the log this block fails:
id = "textarea";
gQueue.push(new synthClick(id, new caretMoveChecker(0, id)));
gQueue.push(new synthRightKey(id, new caretMoveChecker(1, id)));
gQueue.push(new synthDownKey(id, new caretMoveChecker(12, id)));

synthDownKey puts the caret at the end instead the beginning, which is weird. Do you have mac to debug it?

Comment 15

[Samuel Thibault]

I don't have a mac to debug, no.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Samuel Thibault from comment #15)
> I don't have a mac to debug, no.

putting helpwanted into keywords; if no hero arrives, I will pick it up myself when I have cycles.

Jamie, how much we care of os x these days? Should we accept a fix that improves linux and windows, but regresses os x for some unknown reason?

Comment 17

[alexander :surkov (:asurkov)]

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

cancelling review request, until os x is solved or we agree to regress mac

Comment 18

[James Teh [:Jamie]]

(In reply to alexander :surkov (:asurkov) from comment #16)
> Jamie, how much we care of os x these days? Should we accept a fix that
> improves linux and windows, but regresses os x for some unknown reason?

As I understand it, our Mac support is currently broken in some pretty major ways; certainly, no one is actively working on or even dogfooding it. I want to fix this eventually, but it's not currently on the roadmap and I have some work to do to determine whether we can realistically make it a priority. For now, I think you can afford to regress Mac for this case, obviously adjusting the tests accordingly so we don't bust things.

Comment 19

[Samuel Thibault]

Attachment: offsettodompoint

Here is the additional change needed to fix the offset = 1 case. HyperTextAccessible::OffsetToDOMPoint was getting everything wrong because list bullets are not made eTextLeafType (I guess that is because of what is explained in the comment of nsAccUtils::TextLength), and thus OffsetToDOMPoint would compute offset as if it was an embedded object, which it is not (it is actually mere "• " leaf).  It showed up successful on Linux on https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcee3e28c449b37b3d3a05f303b55d3c2f59914c , it's being tried on other platforms on https://treeherder.mozilla.org/#/jobs?repo=try&revision=ead8123c9d995697c3fd660d24ddc0e92bed3018

Comment 20

[Samuel Thibault]

Attachment: setcaretoffset

Here is the updated patch which does check with offset=1. I had to add a parameter to setCaretOffset because setting the offset to 0 or 1 (i.e. within the bullet) actually sets it to 2 (as does happen when using caret browsing, actually).

Applying the two patches is getting tested on https://treeherder.mozilla.org/#/jobs?repo=try&revision=7086faa4aef768b242b6b63ec2f472b5bd9cda72

So, do I add ifs to ignore the MacOS failures? There are quite a few of type 

    accessible/tests/mochitest/events/test_focus_general.html | Can't get accessible for [ 'document node', address: [object HTMLDocument] ]

:/

Comment 21

[Samuel Thibault]

Attachment: offsettodompoint

Oops, sorry, it shouldn't have been the LISTITEM role in the additional change, but STATICTEXT. Here is the fixed patch, being tested alone on https://treeherder.mozilla.org/#/jobs?repo=try&revision=29748fc28f3be5b642152b93517bce18a4008e29

Comment 22

[Samuel Thibault]

And applying the two patches is getting tested on https://treeherder.mozilla.org/#/jobs?repo=try&revision=26e86ab5bac2107ca841d57d74a31e5c9400849b

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 9004930 [details] [diff] [review]
offsettodompoint

Samuel, I will redirect review request to Jamie if you don't mind and of course if Jamie doesn't mind :), since he had played around this code lately, and might have some good ideas.

Comment 24

[Samuel Thibault]

Sure, no problem :

Comment 25

[James Teh [:Jamie]]

Comment on attachment 9004930 [details] [diff] [review]
offsettodompoint

Thanks. Looks fine to me, but please update the commit message as follows:
1. Specify "Bug 1478964 part 1" in the first line. Part 2 will cause test failures without this part, so we should make the ordering clear.
2. In the body of the commit message, include a brief summary of *why* this change is being made; i.e. that otherwise, offset 1 in a bullet node was handled incorrectly. I'm not exactly sure what the incorrect behaviour was (though I do understand the logic was wrong), but it might also be helpful to know what was (incorrectly) happening before the change.

Comment 26

[James Teh [:Jamie]]

(In reply to alexander :surkov (:asurkov) from comment #14)
> synthDownKey puts the caret at the end instead the beginning, which is
> weird. Do you have mac to debug it?

Apparently, on the Mac, down arrow (when not at the end of the line) moves to the end of the line (like the end key in Windows/Linux). So I guess this is expected on the Mac.

Comment 27

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #26)
> Apparently, on the Mac, down arrow (when not at the end of the line) moves
> to the end of the line (like the end key in Windows/Linux).

Oh. This only happens when on the last line (or in a single line input). So I'm probably wrong about this being expected. Sorry for the noise.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #26)
> (In reply to alexander :surkov (:asurkov) from comment #14)
> > synthDownKey puts the caret at the end instead the beginning, which is
> > weird. Do you have mac to debug it?
> 
> Apparently, on the Mac, down arrow (when not at the end of the line) moves
> to the end of the line (like the end key in Windows/Linux). So I guess this
> is expected on the Mac.

I misworded it. This line is failing:

gQueue.push(new synthDownKey(id, new caretMoveChecker(12, id)));

and the failure is

TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/events/test_caretmove.html | Wrong caret offset for ['textarea@id="textarea" node', address: [object HTMLTextAreaElement], role: entry, address: [xpconnect wrapped nsIAccessible]] - got +0, expected 12

so we expected the caret at the end, but it was moved to the beginning

https://treeherder.mozilla.org/logviewer.html#?job_id=192517547&repo=try

Comment 29

[Samuel Thibault]

(FI as the backlog shows, ATM I don't have the resources to work on this)