949518 - Incorrect caret offset if queried while accessible tree is mutating (affects changing value with arrow keys in input type="number")

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
0. Start NVDA with a 2013-12-12 nightly build or later.
1. Find an input type="number". I used this: http://html5doctor.com/demos/forms/forms-example.html
2. Focus on it. I used the Shoesize input.
3. Press Enter to enter focus mode.
4. Press up and down arrows to change the value.

Expected: New value should be announced. I see it changing on the braille display, but NVDA says "blank". The change I see probably comes from just the text in the textbox.
Actual: NVDA says "blank" whenever the value changes.

This should be tracked for both Firefox 29 and 28. 28 is affected once bug 559761 lands on Aurora (see approval request there). This is a missing implementation detail that should also be uplifted for completeness once fixed in 29.

Comment 1

[alexander :surkov (:asurkov)]

I think this is because ValueChange event is not fired for input field inside the numberbox.

Jonathan, would you like to take this?

Comment 2

[Jonathan Watt [:jwatt]]

I don't know anything about the ValueChange event, but 'input' and 'change' events are fired. The 'input' event is fired for almost any change, whereas the 'change' event is fired less frequently (on keyup, mouseup or blur).

I would imagine that the a11y code needs to listen for 'input' events, but I don't know. If so, then an a11y person would be best to work on this. If not, I'm on PTO, starting much later than intended, so I won't be working on this until January I'm afraid.

Comment 3

[Jonathan Watt [:jwatt]]

Seems there is such a thing as a "ValueChange" event, so CC'ing smaug in case he knows why this isn't firing.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Hmm, where do we dispatch ValueChange? It that dispatched for type="range"?

Comment 5

[alexander :surkov (:asurkov)]

I was wrong, we generate the accessible value change event for input field on purely a11y side (we watch for text changes for that).

Marco, btw, I can see a11y valuechange event when up/down arrowing the number box.

Comment 6

[alexander :surkov (:asurkov)]

it'd be good to have Jamie to take a look at the problem

Comment 7

[Marco Zehe (:MarcoZ)]

Definitely, since I definitely don't hear the new value when arrowing up and down when focused on the Number input field in focus mode with NVDA. I hear the new number when tabbing away and back, so the new value is definitely recognised then, but not when arrowing.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Will track this implementation detail so we don't ship it with bug 559761 but we need an assignee here - who can take this?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Lukas Blakk [:lsblakk] from comment #8)
> Will track this implementation detail so we don't ship it with bug 559761
> but we need an assignee here - who can take this?

we don't have enough info to assign the bug

Comment 10

[James Teh [:Jamie]]

In NVDA, we have code for handling the cursor keys in editable text fields. We can't just use caret events because we don't have enough context; e.g. whether the user moved by line, word, character, etc. In general terms, our code:
1. Intercepts the key.
2. Gets the position of the caret.
3. Sends the key.
4. Waits for the caret to move by frequently comparing the position in step 2 with the current caret position.
5. Once the caret moves, it speaks the desired unit at the caret; in this case, the line.

The problem occurs in step 4. Generally, the caret is at the end of the line unless the user changes this. In the shoe size example, if the size "13" is selected, the caret position will be 2 in step 2. In step 4, very early, the caret position reported by Mozilla is 0. NVDA sees that the caret has moved and proceeds to step 5. Unfortunately, at this point, the text is empty; nCharacters returns 0. So, all you get is "blank".

In short, at some point when you press up/downArrow, the field is empty, at least according to a11y. This is detected as caret movement by NVDA, so we quit waiting too early. I'm not really sure how we can work around this, though. We don't know when the field has finished updating.

Comment 11

[James Teh [:Jamie]]

Err, sorry about changing the tracking flags. I've had this tab open for weeks meaning to respond to this bug, and apparently, the form entries were a bit stale. :)

Comment 12

[alexander :surkov (:asurkov)]

Jamie, should we close this bug on our side? or is there anything we can fix to help NVDA?

Comment 13

[James Teh [:Jamie]]

Why does the field go blank briefly? Is that just what happens internally? Do you have any knowledge that another real change is coming? It is the field going blank before the new value arrives that is causing us trouble.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #13)
> Why does the field go blank briefly? Is that just what happens internally?
> Do you have any knowledge that another real change is coming? It is the
> field going blank before the new value arrives that is causing us trouble.

I don't see that either visually or on accessibility events layer. However the guess is
1) when you press up arrow then old text is removed (hide event is fired)
2) text is inserted (show event is fired)
3) caret change event is fired (because the change is in form of 13 - > 13.5

So if you check the caret offset right after hide event but before show event then you can observe wrong caret offset and no text.

Comment 15

[James Teh [:Jamie]]

Hmm. I understand, but it's awkward because the caret offset becoming 0 can't be interpreted in any other way than caret movement. If there is a hide and a show, What prevents two caret events from being fired: event coalescence?

Our cursor movement code can't rely on events alone because some implementations don't fire them or fire them too slowly. I guess this can be wontfixed, but unfortunately, I don't really have a good solution for this problem on our side either.

Comment 16

[Marco Zehe (:MarcoZ)]

Is Gecko's implementation in some way deviating from other spin boxes in Windows? If so, maybe we need to adjust our event firing accordingly. Or does NVDA have a similar problem with other spin boxes as well, jamie?

Comment 17

[James Teh [:Jamie]]

Err, embarrassingly, I can't think of any other spin boxes to test right now. I suspect a lot of them don't bother to implement editable text support at all.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #15)
> Hmm. I understand, but it's awkward because the caret offset becoming 0
> can't be interpreted in any other way than caret movement. If there is a
> hide and a show, What prevents two caret events from being fired: event
> coalescence?

we fire caret move event when caret was moved, in this case hide, show event pair means a text update. I'm sure that in some cases you get get the tree out of sync which is unfortunate. In this case it should be reasonable to fire caret event. I'm curious though if it won't be confusing since it doesn't really correspond to visual state because hide, show are on the same queue - so it's like a single change. How does it happen you get in the middle of show/hide events? Or does something else happen?

Comment 19

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #18)
> we fire caret move event when caret was moved, in this case hide, show event
> pair means a text update. I'm sure that in some cases you get get the tree
> out of sync which is unfortunate. In this case it should be reasonable to
> fire caret event.
Sorry if I wasn't clear. I don't want another caret event. I was just asking how it is possible to prevent the extraneous event, but we can still see the tree out of sync. In other words, why does the tree get out of sync with events?

> I'm curious though if it won't be confusing since it
> doesn't really correspond to visual state because hide, show are on the same
> queue - so it's like a single change.
Right. That's why NVDA gets confused now, except we don't use events for this.

> How does it happen you get in the
> middle of show/hide events?
As I said, we can't use events for our cursor movement code. We send the key and start polling the caret offset.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #19)

> Sorry if I wasn't clear. I don't want another caret event. I was just asking
> how it is possible to prevent the extraneous event, but we can still see the
> tree out of sync. In other words, why does the tree get out of sync with
> events?

I think I lost context. But in general events are fired *after* mutation, so right *before* the event the tree may be different.

> > How does it happen you get in the
> > middle of show/hide events?
> As I said, we can't use events for our cursor movement code. We send the key
> and start polling the caret offset.

can you check at what time do you ask for caret offset? is it after/before show/hide events? It sounds like you shouldn't get 0 offset.

Comment 21

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #20)
> can you check at what time do you ask for caret offset? is it after/before
> show/hide events?
It looks like it occurs before we receive any events, but out-proc win events are async, so I wouldn't trust that in relation to when Mozilla receives the call. We make the first call as soon as the key is pressed and then three times at 10 ms intervals thereafter, but again, out-proc makes those times inaccurate.

Comment 22

[alexander :surkov (:asurkov)]

If you realize one day you want some tricky debugging then I would recommend this one (I would try to reproduce this in js via setTimeout thing).

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

what's the status of this now?  is making this work something that still needs to be fixed in Firefox code or NVDA?

Comment 24

[alexander :surkov (:asurkov)]

(In reply to Lukas Blakk [:lsblakk] from comment #23)
> what's the status of this now?  is making this work something that still
> needs to be fixed in Firefox code or NVDA?

somebody should try to get a test case

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

We're out of time for the FF28 cycle.  Perhaps having the wider user base (and reaching more people browser with a11y features) will get us a better test case to help with fixing for FF29.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Any chance someone could work on this issue for 29? Thanks

Comment 27

[Jonathan Wei [:jwei]]

Attachment: Cache caret offset and update on caret move event

To avoid NVDA receiving invalid caret information if an offset is requested while the accessible tree is still being updated, we cache the last caret offset and update it when the caret is set explicitly (SetCaretOffset) or a caret move event is fired (from user input).

Comment 28

[alexander :surkov (:asurkov)]

Comment on attachment 8402728 [details] [diff] [review]
Cache caret offset and update on caret move event

Review of attachment 8402728 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/HyperTextAccessible.h
@@ +302,5 @@
>    /**
>     * Get/set caret offset, if no caret then -1.
>     */
> +  int32_t CaretOffset();
> +  int32_t GetCaretOffset();

something weird with these methods, at least with their names

@@ +532,5 @@
>     * End text offsets array.
>     */
>    mutable nsTArray<uint32_t> mOffsets;
> +  // Cached caret offset.
> +  int32_t mCaretOffset;

cached offset in hypertext is bad for memory

caret always follows the focus, so it's enough to have an unique generic variable to cache the caret offset relative some accessible, caret offsets relative other accessibles should be calculated on request, say you have

<body>
  <div contentEditable="true">hello</div>
</body>

div is focused and caret after 'e', so div accessible has 2 caret offset (cached value), document has 1 caret offset (calculated value).

Comment 29

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #28)
> cached offset in hypertext is bad for memory
> 
> caret always follows the focus, so it's enough to have an unique generic
> variable to cache the caret offset relative some accessible, caret offsets
> relative other accessibles should be calculated on request, say you have
> 
> <body>
>   <div contentEditable="true">hello</div>
> </body>
> 
> div is focused and caret after 'e', so div accessible has 2 caret offset
> (cached value), document has 1 caret offset (calculated value).

Okay.  Would it make sense to put the variable and related calculation/updating methods in FocusManager?

Comment 30

[alexander :surkov (:asurkov)]

or SelectionManager

Comment 31

[Jonathan Wei [:jwei]]

Attachment: Cache caret offset and update on caret move event v2

I moved the cached offset from each individual HyperTextAccessible to a single variable in SelectionManager.  The currently focused accessible uses it, keeping it updated on caret move events.  If the caret offset is requested for an unfocused accessible, it is calculated, ignoring the cached value.  When the focus changes, the cached value is cleared to be recalculated on the next access.

Is this approach closer to what you had in mind?

Comment 32

[alexander :surkov (:asurkov)]

Comment on attachment 8403379 [details] [diff] [review]
Cache caret offset and update on caret move event v2

Review of attachment 8403379 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/SelectionManager.cpp
@@ +229,5 @@
>    }
>  }
> +
> +int32_t
> +SelectionManager::GetCaretOffset(Accessible* aItem)

I think I would have AccessibleWithCaret (similar to http://accessibility.linuxfoundation.org/a11yspecs/ia2/api/Accessible2_2.idl) and make HyperText accessible to calculate the caret offset based on it (also see bug 873438)

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +1194,5 @@
> +
> +void
> +HyperTextAccessible::SetCaretOffset(int32_t aOffset)
> +{
> +  SetSelectionRange(aOffset, aOffset);

shouldn't it trigger selection change events and thus a caret update?

keep them inline?

::: accessible/src/generic/HyperTextAccessible.h
@@ +300,5 @@
>  
>    /**
>     * Get/set caret offset, if no caret then -1.
>     */
> +  int32_t GetCaretOffset();

why do you rename it?

Comment 33

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #32)
> I think I would have AccessibleWithCaret (similar to
> http://accessibility.linuxfoundation.org/a11yspecs/ia2/api/Accessible2_2.
> idl) and make HyperText accessible to calculate the caret offset based on it
> (also see bug 873438)

I'm not sure I understand.  Reading over the interface, it's useful for retrieving an offset given an accessible that isn't necessarily a hypertext accessible.  In this case, the only accessibles calling GetCaretOffset are already of type HyperTextAccessible.  Should I just make it explicit in taking an argument of type HyperTextAccessible?  I'm not sure how implementing the AccessibleWithCaret interface would help for this use case.

If I'm completely wrong in what you meant, then clarification would be great.

> shouldn't it trigger selection change events and thus a caret update?
> 
> keep them inline?

It does trigger a caret update, but too late.  Currently, the cached value is updated when a caret move event is processed.  In accessible/src/jsat/ContentControl.jsm:217, handleActivate sets the caret offset and then calls caretOffset() immediately after. This still returns the old cached value, since the event is handled later.

It's out of line because it's using a method in SelectionManager to update the cached value immediately, and I didn't want to dump the SelectionManager into the HyperTextAccessible header file.

> why do you rename it?

I renamed them to Get/Set/Calculate to reflect what the methods do.  Previously, it was CaretOffset/SetCaretOffset, which was fine when it was always calculated on demand.  Now, CalculateCaretOffset explicitly calculates the offset where GetCaretOffset may retrieve the cached value instead.

Comment 34

[alexander :surkov (:asurkov)]

(In reply to Jonathan Wei [:jwei] from comment #33)
> (In reply to alexander :surkov from comment #32)
> > I think I would have AccessibleWithCaret (similar to
> > http://accessibility.linuxfoundation.org/a11yspecs/ia2/api/Accessible2_2.
> > idl) and make HyperText accessible to calculate the caret offset based on it
> > (also see bug 873438)
> 
> I'm not sure I understand.  Reading over the interface, it's useful for
> retrieving an offset given an accessible that isn't necessarily a hypertext
> accessible.  In this case, the only accessibles calling GetCaretOffset are
> already of type HyperTextAccessible.  Should I just make it explicit in
> taking an argument of type HyperTextAccessible?  I'm not sure how
> implementing the AccessibleWithCaret interface would help for this use case.

I meant SelectionManager has
int32_t caretOffset AccessibleWithCaret(HyperTextAccessible** aText);

then HyperTextAccessible has CaretOffset() method that transform the result of AccWithCaret relative the given accessible 

> It does trigger a caret update, but too late.  Currently, the cached value
> is updated when a caret move event is processed.  In
> accessible/src/jsat/ContentControl.jsm:217, handleActivate sets the caret
> offset and then calls caretOffset() immediately after. This still returns
> the old cached value, since the event is handled later.

mm, ok

> It's out of line because it's using a method in SelectionManager to update
> the cached value immediately, and I didn't want to dump the SelectionManager
> into the HyperTextAccessible header file.

we have HyperTextAccessible-inl.h file

> > why do you rename it?
> 
> I renamed them to Get/Set/Calculate to reflect what the methods do. 
> Previously, it was CaretOffset/SetCaretOffset, which was fine when it was
> always calculated on demand.  Now, CalculateCaretOffset explicitly
> calculates the offset where GetCaretOffset may retrieve the cached value
> instead.

I think we get rid 'Get' from getters in general. But let's see I think we don't need to have two methods here at all

Comment 35

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #34)
> I meant SelectionManager has
> int32_t caretOffset AccessibleWithCaret(HyperTextAccessible** aText);
> 
> then HyperTextAccessible has CaretOffset() method that transform the result
> of AccWithCaret relative the given accessible 

Okay, that makes sense.  I'll do that.

> we have HyperTextAccessible-inl.h file

I forgot about the existence of that file - I'll move it over there.

> I think we get rid 'Get' from getters in general. But let's see I think we
> don't need to have two methods here at all

Yeah, I'll merge the two and just return early if we're using the cached value from SelectionManager.

Comment 36

[Jonathan Wei [:jwei]]

Attachment: Cache caret offset and update on caret move event v3 WIP

WIP based on feedback on version 2 of the patch.

Currently fails the following 3 accessibility mochitests:

 0:26.50 4 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_caretmove.html | Wrong caret offset for ['input@id="textbox" node', address: [object HTMLInputElement], role: entry, address: 0x13247ef80] - got 0, expected 5

 0:26.50 48 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_caretmove.html | Wrong caret offset for ['textarea@id="textarea" node', address: [object HTMLTextAreaElement], role: entry, address: 0x13247f0e0] - got 0, expected 12

 0:26.50 99 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/test_content_text.html | 0 [ brailleOutput [ selectionStart [ expected 0 got 59 ] selectionEnd [ expected 0 got 59 ] ] ]

Comment 37

[alexander :surkov (:asurkov)]

Comment on attachment 8404107 [details] [diff] [review]
Cache caret offset and update on caret move event v3 WIP

Review of attachment 8404107 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/FocusManager.cpp
@@ +363,5 @@
>    if (logging::IsEnabled(logging::eFocus))
>      logging::FocusNotificationTarget("fire focus event", "Target", target);
>  #endif
>  
> +  SelectionMgr()->ResetCaretOffset();

are you sure there's always selection change event after focus is changed? or how do we update the caret offset?

::: accessible/src/base/SelectionManager.cpp
@@ +36,5 @@
>    ~SelData() {}
>  };
>  
> +SelectionManager::SelectionManager() :
> +  mCaretOffset(-1), mCaretAccessible(nullptr)

maybe it's good to name it like mAccWithCaret because on MSAA there's an accessible object for caret

@@ +155,2 @@
>    if (caretOffset != -1) {
> +    if (FocusMgr()->IsFocused(event->GetAccessible())) {

why do you check focus here? it shouldn't work in case of

<div contentEditable="true">
  <p>type text here</p>
</div>

p is target of caret change events but it's never focused

@@ +236,5 @@
> +int32_t
> +SelectionManager::AccessibleWithCaret(HyperTextAccessible** aText)
> +{
> +  *aText = mCaretAccessible;
> +  return mCaretOffset;

it makes sense to return HyperTextAccessible pointer as return value, otherwise we may run into addref/release assumptions confusion (sorry for not saying it earlier)

@@ +243,5 @@
> +void
> +SelectionManager::UpdateCaretOffset(Accessible* aItem, int32_t aOffset)
> +{
> +  if (FocusMgr()->IsFocused(aItem))
> +    mCaretOffset = aOffset;

you don't change accessible with caret

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +1158,5 @@
> +    if (text == this)
> +      return caretOffset;
> +
> +    if (IsDoc())
> +      return 0;

it's strange because in case of

<body>hello hello</body>

the document text is 'hello hello'.

@@ +1161,5 @@
> +    if (IsDoc())
> +      return 0;
> +
> +    if (text)
> +      return TransformOffset(text, caretOffset, false);

does it work properly when this is not a child of text object?

Comment 38

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #37)
> are you sure there's always selection change event after focus is changed?
> or how do we update the caret offset?

If SelectionManager::AccessibleWithCaret doesn't return useful information, then HyperTextAccessible::CaretOffset does the regular calculation it used to before.  

> maybe it's good to name it like mAccWithCaret because on MSAA there's an
> accessible object for caret

Okay.

> why do you check focus here? it shouldn't work in case of
> 
> <div contentEditable="true">
>   <p>type text here</p>
> </div>
> 
> p is target of caret change events but it's never focused

The focus check is for updating the cache.  The event itself is still fired.

> it makes sense to return HyperTextAccessible pointer as return value,
> otherwise we may run into addref/release assumptions confusion (sorry for
> not saying it earlier)

Okay, so the signature would now be HyperTextAccessible* AccessibleWithCaret(int32_t& aCaret)?

> you don't change accessible with caret

Yes, HyperTextAccessible::SetCaretOffset changes focus if needed anyway, so the accessible should be set here as well.

> it's strange because in case of
> 
> <body>hello hello</body>
> 
> the document text is 'hello hello'.

This part of the logic is incomplete and was placed there solely to pass tests.  Is there a reference to offset calculation if the accessible is a document?

> does it work properly when this is not a child of text object?

Oops, I forgot to make sure the cached accessible is actually a descendant.  I'll fix up the logic there.

Comment 39

[alexander :surkov (:asurkov)]

(In reply to Jonathan Wei [:jwei] from comment #38)

> > <div contentEditable="true">
> >   <p>type text here</p>
> > </div>
> > 
> > p is target of caret change events but it's never focused
> 
> The focus check is for updating the cache.  The event itself is still fired.

right but you need to update the cache, agree?

> Okay, so the signature would now be HyperTextAccessible*
> AccessibleWithCaret(int32_t& aCaret)?

right

> > you don't change accessible with caret
> 
> Yes, HyperTextAccessible::SetCaretOffset changes focus if needed anyway, so
> the accessible should be set here as well.

tricky, wish we had something more explicit

> > it's strange because in case of
> > 
> > <body>hello hello</body>
> > 
> > the document text is 'hello hello'.
> 
> This part of the logic is incomplete and was placed there solely to pass
> tests.  Is there a reference to offset calculation if the accessible is a
> document?

I'm not sure if you need special logic for document at all. But I'm not sure I follow your question.

Comment 40

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #39)
> right but you need to update the cache, agree?

I don't think so.  The cache shadows the currently focused accessible, so if the target of the caret change events isn't focused, then we shouldn't update the cache.

> I'm not sure if you need special logic for document at all. But I'm not sure
> I follow your question.

Given an accessible (not a document) and offset, how does the document that contains the accessible calculate its caret offset?

Comment 41

[alexander :surkov (:asurkov)]

(In reply to Jonathan Wei [:jwei] from comment #40)
> (In reply to alexander :surkov from comment #39)
> > right but you need to update the cache, agree?
> 
> I don't think so.  The cache shadows the currently focused accessible, so if
> the target of the caret change events isn't focused, then we shouldn't
> update the cache.

ok, say the user navigates that paragraph text and screen reader request the caret offset for the paragraph accessible, what is return value in this case?

> > I'm not sure if you need special logic for document at all. But I'm not sure
> > I follow your question.
> 
> Given an accessible (not a document) and offset, how does the document that
> contains the accessible calculate its caret offset?

the rule is if caret is inside an embedded object then caret offset relative parent (document in your case) is after the embedded character. For example

<body><p>hello</p></body>

if caret offset is after 'l' character then caret offset relative document is 1 (p is represented by embedded character, document text is embedded character only). you can check out TranformOffset logic for that.

Comment 42

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #41)
> the rule is if caret is inside an embedded object then caret offset relative
> parent (document in your case) is after the embedded character. For example
> <body><p>hello</p></body>
> if caret offset is after 'l' character then caret offset relative document
> is 1 (p is represented by embedded character, document text is embedded
> character only).
I think the caret offset in the document should be 0 (the embedded object), not 1. Otherwise, you can't tell where the caret is when traversing from the document. See bug 672717 comment 3.

Comment 43

[Jonathan Wei [:jwei]]

Attachment: Cache caret offset and update on caret move event v4

Comment 44

[alexander :surkov (:asurkov)]

Comment on attachment 8405572 [details] [diff] [review]
Cache caret offset and update on caret move event v4

Review of attachment 8405572 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/FocusManager.cpp
@@ +363,5 @@
>    if (logging::IsEnabled(logging::eFocus))
>      logging::FocusNotificationTarget("fire focus event", "Target", target);
>  #endif
>  
> +  SelectionMgr()->ResetCaretOffset();

it'd be good to have a comment here. Do we always receive subsequent text selection change after focus?

::: accessible/src/base/SelectionManager.cpp
@@ +151,5 @@
>      return;
>  
> +  Selection* selection = caretCntr->DOMSelection();
> +  int32_t caretOffset = caretCntr->DOMPointToOffset(selection->GetFocusNode(),
> +    selection->FocusOffset());

nit: you don't need caretOffset variable, also pls align function arguments

::: accessible/src/base/SelectionManager.h
@@ +82,5 @@
>     */
>    void ProcessTextSelChangeEvent(AccEvent* aEvent);
>  
> +  /**
> +   * Gets the current caret offset/hypertext accessible pair.  If there is no

nit: two spaces before If

@@ +86,5 @@
> +   * Gets the current caret offset/hypertext accessible pair.  If there is no
> +   * current pair, then returns -1 for the offset and a nullptr for the
> +   * accessible.
> +   */
> +  HyperTextAccessible* AccessibleWithCaret(int32_t& aCaret);

pls do int32_t*

::: accessible/src/generic/HyperTextAccessible-inl.h
@@ +42,5 @@
> +inline void
> +HyperTextAccessible::SetCaretOffset(int32_t aOffset)
> +{
> +  SetSelectionRange(aOffset, aOffset);
> +  SelectionMgr()->UpdateCaretOffset(this, aOffset);

I'm curious what if we failed to set up the caret at the given offset. How many test/accessfu fails, it makes sense to consider to fix their expectations (we cannot guarantee sync processing)

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +1160,5 @@
>    // is not inside of focused node.
>    FocusManager::FocusDisposition focusDisp =
>      FocusMgr()->IsInOrContainsFocus(this);
>    if (focusDisp == FocusManager::eNone)
>      return -1;

it looks like we can avoid this tree traversal having an accessible with caret (since it reflects where the focus is)

@@ +1163,5 @@
>    if (focusDisp == FocusManager::eNone)
>      return -1;
>  
> +  // Check cached value.
> +  HyperTextAccessible* text = nullptr;

pls define where it's used

Comment 45

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #44)
> it'd be good to have a comment here. Do we always receive subsequent text
> selection change after focus?

In the case that the focus isn't a text field, then not necessarily.  If the focus was moved to a text field, it seems that a text caret move event follows in the event queue.

> pls do int32_t*

Okay.

> I'm curious what if we failed to set up the caret at the given offset. How
> many test/accessfu fails, it makes sense to consider to fix their
> expectations (we cannot guarantee sync processing)

Hmm.  At least from jsat/ContentControl.jsm there's a range check on the set caret offset to make sure it will work (>= 0 and <= character count).  From all other call sites, there's a check to ensure that the caret offset value is valid, other than xpcAccessibleHyperText::SetScriptableCaretOffset which is what was used.  Does it make sense to add range checking on that as well, so that setting the caret won't fail?

> it looks like we can avoid this tree traversal having an accessible with
> caret (since it reflects where the focus is)

Yeah.  I'll rearrange the logic so that if we have a cached value from AccessibleWithCaret, we don't go through IsInOrContainsFocus.

> pls define where it's used

I'm not sure what you mean by this - there's a few comments in the logic below.  Do you mean adding more detail to the initial comment?

Comment 46

[alexander :surkov (:asurkov)]

(In reply to Jonathan Wei [:jwei] from comment #45)
> (In reply to alexander :surkov from comment #44)
> > it'd be good to have a comment here. Do we always receive subsequent text
> > selection change after focus?
> 
> In the case that the focus isn't a text field, then not necessarily.  If the
> focus was moved to a text field, it seems that a text caret move event
> follows in the event queue.

do I understand right this is a trick to return correct caret offset when it was asked right after focus event but before caret move event?

> > I'm curious what if we failed to set up the caret at the given offset. How
> > many test/accessfu fails, it makes sense to consider to fix their
> > expectations (we cannot guarantee sync processing)
> 
> Hmm.  At least from jsat/ContentControl.jsm there's a range check on the set
> caret offset to make sure it will work (>= 0 and <= character count).  From
> all other call sites, there's a check to ensure that the caret offset value
> is valid, other than xpcAccessibleHyperText::SetScriptableCaretOffset which
> is what was used.  Does it make sense to add range checking on that as well,
> so that setting the caret won't fail?

it's worth to ask Yura on it. But maybe we can leave it for now.

> > pls define where it's used
> 
> I'm not sure what you mean by this - there's a few comments in the logic
> below.  Do you mean adding more detail to the initial comment?

sorry I meant to do
T* t = Method();
instead
T* t = 0;
t = Method();

Comment 47

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #46)
> do I understand right this is a trick to return correct caret offset when it
> was asked right after focus event but before caret move event?

Yes.  In the case that the caret move event hasn't been processed yet to safely update the cache, it'll go through the original on-demand calculations.  I'll write a comment there for clarity.

> sorry I meant to do
> T* t = Method();
> instead
> T* t = 0;
> t = Method();

Oops, didn't notice I had written the code like that.  I'll fix it.

Comment 48

[alexander :surkov (:asurkov)]

Comment on attachment 8405572 [details] [diff] [review]
Cache caret offset and update on caret move event v4

it'd be good to take a look at next patch

Comment 49

[Yura Zenevich [:yzen] (please use "needinfo")]

It looks like we have a special case for an ENTRY role where we present, if necessary, the caret change synchronously. I think a proper approach would be presenting the caret change with the actual caret moved event. There should be no need to do the trick on your side and we should be able to remove the one on ours.

Comment 50

[Jonathan Wei [:jwei]]

Attachment: Added old caret offset to caret move event, AccessFu changes

The nsIAccessibleCaretMoveEvent now includes the old caret offset needed by jsat, and presentCaretChange was removed from ContentControl to use the regular accessibility events in EventManager instead.  However, this has a likely unwanted side effect of making the events appear to be not generated by the user, whereas presentCaretChange impersonated the user.  This results in improper Android event firing.  Is there a better approach to this, or should the original logic in ContentControl be kept and just refresh the cached caret offset in HyperTextAccessible::SetCaretOffset?

Comment 51

[Eitan Isaacson [:eeejay]]

(In reply to Jonathan Wei [:jwei] from comment #50)
> Created attachment 8408312 [details] [diff] [review]
> Added old caret offset to caret move event, AccessFu changes
> 
> The nsIAccessibleCaretMoveEvent now includes the old caret offset needed by
> jsat, and presentCaretChange was removed from ContentControl to use the
> regular accessibility events in EventManager instead.  However, this has a
> likely unwanted side effect of making the events appear to be not generated
> by the user, whereas presentCaretChange impersonated the user.  This results
> in improper Android event firing.  Is there a better approach to this, or
> should the original logic in ContentControl be kept and just refresh the
> cached caret offset in HyperTextAccessible::SetCaretOffset?

As I understand it, isFromUserInput is false on the event fired after the caretOffset is changed. Correct?

I believe that isFromUserInput is inaccurately false in this case. Any text modification or caret change via the accessible text interface should be considered user input, since the AT is doing it as an agent of the user.

isFromUserInput should only be false when the change is from content itself, like js or a css rule change.

Comment 52

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #51)

> I believe that isFromUserInput is inaccurately false in this case. Any text
> modification or caret change via the accessible text interface should be
> considered user input, since the AT is doing it as an agent of the user.
> 
> isFromUserInput should only be false when the change is from content itself,
> like js or a css rule change.

API can be used by anyone but API is not really user input either case. Curious what Jamie thinks.

Comment 53

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #52)
> > I believe that isFromUserInput is inaccurately false in this case. Any text
> > modification or caret change via the accessible text interface should be
> > considered user input, since the AT is doing it as an agent of the user.
> > isFromUserInput should only be false when the change is from content itself,
> > like js or a css rule change.
> API can be used by anyone but API is not really user input either case.
> Curious what Jamie thinks.
Tricky. An AT might move the caret, for example, when the user presses cursor routing buttons on a braille display or moves the cursor using screen reader gestures on a touch screen. As Eitan says, the AT is doing it as an agent of the user; the user explicitly requested that behaviour. On the other hand, the AT might want to be able to distinguish such cases from user input it did not trigger. I'm leaning towards the former (Eitan's) perspective, though. If an AT really did want to distinguish API triggered events, it'd probably need another concept like wasTriggeredByApi or something.

Comment 54

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #53)
> If an AT really did want to distinguish API triggered
> events, it'd probably need another concept like wasTriggeredByApi or
> something.

Agree, I was curious if it can be a privacy issue. Otherwise I'm fine to go with Eitan suggestion.

Comment 55

[Jonathan Wei [:jwei]]

Attachment: Cache caret offset and update on caret move event v5

Force a cache refresh of the caret offset until there's a nice implementation for emulating user input.

Comment 56

[alexander :surkov (:asurkov)]

Comment on attachment 8409749 [details] [diff] [review]
Cache caret offset and update on caret move event v5

Review of attachment 8409749 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/FocusManager.cpp
@@ +365,5 @@
>  #endif
>  
> +  // Reset cached caret value. The cache will be updated upon processing the next
> +  // caret move event.
> +  SelectionMgr()->ResetCaretOffset();

you didn't say here that this is a trick that allows to return correct caret offset after focus is fired but before caret move event is handled.

::: accessible/src/base/SelectionManager.cpp
@@ +151,5 @@
>      return;
>  
> +  Selection* selection = caretCntr->DOMSelection();
> +  mCaretOffset = caretCntr->DOMPointToOffset(selection->GetFocusNode(),
> +                                             selection->FocusOffset());

just a comment: we still have a chance that DOM tree and accessible tree are out of sync. You can do a simple test case to check that: handle focus on textbox and remove its content. but it's separate issue I think.

@@ +235,5 @@
> +{
> +  if (aCaret)
> +    *aCaret = mCaretOffset;
> +
> +  return mAccWithCaret;

make it inline?

@@ +242,5 @@
> +void
> +SelectionManager::UpdateCaretOffset(HyperTextAccessible* aItem, int32_t aOffset)
> +{
> +  mAccWithCaret = aItem;
> +  mCaretOffset = aOffset;

same

::: accessible/src/base/SelectionManager.h
@@ +111,5 @@
>  private:
>    // Currently focused control.
>    nsWeakFrame mCurrCtrlFrame;
> +  int32_t mCaretOffset;
> +  HyperTextAccessible* mAccWithCaret;

if you ask for accessible with caret on content_removed event if you removed focused textbox from DOM then will you crash?

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +1159,5 @@
> +  int32_t caretOffset = -1;
> +  HyperTextAccessible* text = SelectionMgr()->AccessibleWithCaret(&caretOffset);
> +
> +  if (caretOffset != -1) {
> +    // Use cached value if it corresponds to this accessible.

nit: pls move the comment right before 'if' statement

::: accessible/tests/mochitest/events.js
@@ +1768,5 @@
>      if (aStartOffset == aEndOffset) {
> +      waitForEvent(EVENT_TEXT_CARET_MOVED, aID, function() {
> +        is(getAccessible(aID, [nsIAccessibleText]).caretOffset, aStartOffset,
> +           "Wrong collapsed selection!");
> +      });

there's something wrong with it, I'd say callers should have one more checker for caret move event

Comment 57

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #56)
> if you ask for accessible with caret on content_removed event if you removed
> focused textbox from DOM then will you crash?

Hmm.  The entry point from nsIAccessibleText will first check if the accessible is defunct before proceeding.  From other points, is it sufficient to check if the returned accessible from AccessibleWithCaret is defunct, or does it make more sense to clear the cached offset when an accessible is flagged as defunct?

> there's something wrong with it, I'd say callers should have one more
> checker for caret move event

If you meant that there should be an explicit caret move checker, it's right above the textSelectionChecker function.  The waitForEvent is because events/test_textselchange creates a 1 character selection, decreases it to 0 characters, and immediately checks the caret offset.

Comment 58

[alexander :surkov (:asurkov)]

(In reply to Jonathan Wei [:jwei] from comment #57)
> (In reply to alexander :surkov from comment #56)
> > if you ask for accessible with caret on content_removed event if you removed
> > focused textbox from DOM then will you crash?
> 
> Hmm.  The entry point from nsIAccessibleText will first check if the
> accessible is defunct before proceeding.
>  From other points, is it
> sufficient to check if the returned accessible from AccessibleWithCaret is
> defunct, or does it make more sense to clear the cached offset when an
> accessible is flagged as defunct?

defunct is ok as long as accessible object is alive, but if it was destroyed then it's a problem. the question is if you can get an access to destroyed object via AccessibleWithCaret. I don't see scenario (the accessible is alive on hide event, but accessibleWithCaret will be updated after timeout), so probably we can go with raw reference. 

> > there's something wrong with it, I'd say callers should have one more
> > checker for caret move event
> 
> If you meant that there should be an explicit caret move checker, it's right
> above the textSelectionChecker function.

I meant the invoker should have event sequeance consisting of caretMoveChecker and textSelectionChecker.

Comment 59

[Jonathan Wei [:jwei]]

(In reply to alexander :surkov from comment #58)
> defunct is ok as long as accessible object is alive, but if it was destroyed
> then it's a problem. the question is if you can get an access to destroyed
> object via AccessibleWithCaret. I don't see scenario (the accessible is
> alive on hide event, but accessibleWithCaret will be updated after timeout),
> so probably we can go with raw reference. 

Okay, I'll leave it as is, then.

> I meant the invoker should have event sequeance consisting of
> caretMoveChecker and textSelectionChecker.

Unfortunately, the text selection event gets queued and processed before the text caret moved event.  Since we're using the text caret moved event to update the cache, the sequence will be waiting for an event that has already occurred.

Comment 60

[alexander :surkov (:asurkov)]

(In reply to Jonathan Wei [:jwei] from comment #59)
> (In reply to alexander :surkov from comment #58)
> > defunct is ok as long as accessible object is alive, but if it was destroyed
> > then it's a problem. the question is if you can get an access to destroyed
> > object via AccessibleWithCaret. I don't see scenario (the accessible is
> > alive on hide event, but accessibleWithCaret will be updated after timeout),
> > so probably we can go with raw reference. 
> 
> Okay, I'll leave it as is, then.
> 
> > I meant the invoker should have event sequeance consisting of
> > caretMoveChecker and textSelectionChecker.
> 
> Unfortunately, the text selection event gets queued and processed before the
> text caret moved event. 

and that's fine, you can change the invokers to

this.eventSeq = [
  new textSelChangeChecker(),
  new caretMoveChecker()
];

right?

Comment 61

[Jonathan Wei [:jwei]]

Attachment: Cache caret offset and update on caret move event v6

Edited textSelectionChecker for the collapsed selection case and added caretMoveChecker to events/test_textlselchange.html.

Comment 62

[alexander :surkov (:asurkov)]

Comment on attachment 8412685 [details] [diff] [review]
Cache caret offset and update on caret move event v6

Review of attachment 8412685 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks!

Comment 63

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ed4c52e21a5f

Comment 64

[James Teh [:Jamie]]

Adjusting the summary to more accurately reflect the bug. I hope that's okay.

Comment 65

[James Teh [:Jamie]]

Verified fixed in Firefox 32.0a1 (2014-05-15). Thanks!