1479678 - Text attribute offsets broken in editor for accessibles after first spelling error

Status: RESOLVED FIXED

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

Description

[James Teh [:Jamie]]

Another text bug! :(

STR (with the NVDA screen reader):
1. Open this test case:
data:text/html,<div contentEditable="true"><div>Test tset</div><div>Test tset</div>
2. Focus the contentEditable.
3. Press control+home to read the first line.
Observe: "Test  spelling error  tset"
4. Press down arrow to read the second line.
Expected: "Test  spelling error  tset"
Actual: "Test tset" (no indication of the spelling error)

If you call IAccessibleText::attributes(0) on the second inner div, you get (9, 5). That should be (0, 5).

Impact: This means that spelling errors often aren't reported while editing in Firefox with NVDA; e.g. with Gmail.

Comment 1

[James Teh [:Jamie]]

Attachment: Fix incorrect start offset when retrieving accessible text attributes if the last spelling error is not within this accessible.

When dealing with an editor which contains multiple accessibles, the previous spelling error range might be in a previous accessible, not the accessible currently being queried.
In this case, DOMPointToOffset will return the length of this accessible.
Previously, we weren't checking for this and were overriding the start offset of the returned range regardless, resulting in broken offsets.
Now, we leave the start offset alone in this case.

Unfortunately, while this test does work, it's hacky/unreliable at best. Spell checking happens async, but it seems it can take quite some time.
I tried waiting for a focus event, but that doesn't help.
Since it's happening async, I figured it might fire a text attribute change. However, waiting on that event just causes us to block indefinitely.
Now I use onSpellCheck from AsyncSpellCheckTestHelper, which is supposed to deal with this. However, just waiting for this once doesn't do it. I have to try waiting for this up to 30 times before the test reliably passes. This uses a timer and returns if no spell check start/begin notifications fire before the timeout, which is the only reason that this hack works.
Because of the timer, I tried replacing this with my own code to observe inlineSpellChecker-spellCheck-ended. However, this seems to get fired twice, and even that isn't enough. And if I wait for it a third time, it blocks indefinitely.
Is there something else I should watch for here?
Does a11y somehow cache spell check stuff? From what I can see, it doesn't seem to, but maybe I'm missing something.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 9001938 [details] [diff] [review]
Fix incorrect start offset when retrieving accessible text attributes if the last spelling error is not within this accessible.

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

::: accessible/tests/mochitest/textattrs/test_spelling.html
@@ +26,5 @@
> +
> +      let editable = document.getElementById("div_after_misspelling");
> +      editable.focus();
> +      // fixme: How do we reliably wait for all spell checking to complete?
> +      for (let i = 0; i < 40; ++i) {

that 40 is ugly :) it can be a source of intermittent failures I bet. 

I'm not sure though I catch why await onSpellCheck doesn't work? Because we process spellcheck notifications async in a11y? Should we instead fire text_attr change events or at least it should be better to make an infinite loop - the test will either hang if we have a bug, or it will finish occasionally, but no intermittents.

Comment 3

[James Teh [:Jamie]]

(In reply to alexander :surkov (:asurkov) from comment #2)
> > +      // fixme: How do we reliably wait for all spell checking to complete?
> > +      for (let i = 0; i < 40; ++i) {
> that 40 is ugly :) it can be a source of intermittent failures I bet. 

It sure is, and it sure could. :) The patch certainly can't land like this. I just ran out of things to try and was hoping someone else would have some thoughts.

> I'm not sure though I catch why await onSpellCheck doesn't work?

It seems onSpellCheck uses a timer itself as a fallback and it's obviously taking longer than this for spell checking to be completed. However, as I noted above, even listening for the spell check end notifications doesn't seem to work, so there must be some async stuff even after that notification fires. I get two spell check end notifications, neither of which seems to be accurate, and if I try waiting for a third one, it blocks forever.

> Because we
> process spellcheck notifications async in a11y?

When you say we process them async, is there any state that we build async related to spell checking or are you saying just the events are async? From what I can see, the text attributes code interrogates the DOM selection directly; there's no caching layer or anything like that. Am I perhaps missing something?

> Should we instead fire
> text_attr change events

What's odd is that we don't seem to fire text atr change events in this case.

> or at least it should be better to make an infinite
> loop - the test will either hang if we have a bug, or it will finish
> occasionally, but no intermittents.

I would have thought hanging is just a special case of an intermittent. :) Still, worst case scenario, maybe I can poll the first attribute range until it changes or something like that. Still ugly, but at least it'll be reliable. I'd really prefer a better solution, though.

Comment 4

[James Teh [:Jamie]]

Hi Drew. I'm really hoping you can help me. :) I'm using your onSpellCheck function in editor/AsyncSpellCheckTestHelper.jsm to try to detect when spell checking finishes in a <div contenteditable="true" spellcheck="true">. However, it seems the onSpellCheck timer always elapses before spell checking finishes. The test is for accessibility, but accessibility just gets the DOM selection for eSpellCheck, and it seems it does not contain the misspelled words. If I delay the call longer (e.g. if I wait for onSpellCheck somewhere between 5 and 30 times), things work as expected. Curiously, if I just wait for inlineSpellChecker-spellCheck-ended notifications myself (so as to avoid the timing factor), I get two of them, but things still aren't up to date after the second one... and there's never a third. I looked at some of the other tests, but I don't see anything that should make them behave better that I'm not doing.

Do you have any ideas at all here? Is there some async updating of the real editor selection even after the inlineSpellChecker-spellCheck-ended notifications fire? I'm at a loss. Any thoughts at all you might have would be very much appreciated.

Thanks for your time.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #3)
> (In reply to alexander :surkov (:asurkov) from comment #2)
> > > +      // fixme: How do we reliably wait for all spell checking to complete?
> > > +      for (let i = 0; i < 40; ++i) {
> > that 40 is ugly :) it can be a source of intermittent failures I bet. 
> 
> It sure is, and it sure could. :) The patch certainly can't land like this.
> I just ran out of things to try and was hoping someone else would have some
> thoughts.

right, but why 40? Is it kinda special number or? :)

> > I'm not sure though I catch why await onSpellCheck doesn't work?
> 
> It seems onSpellCheck uses a timer itself as a fallback and it's obviously
> taking longer than this for spell checking to be completed. However, as I
> noted above, even listening for the spell check end notifications doesn't
> seem to work, so there must be some async stuff even after that notification
> fires. I get two spell check end notifications, neither of which seems to be
> accurate, and if I try waiting for a third one, it blocks forever.
> 
> > Because we
> > process spellcheck notifications async in a11y?
> 
> When you say we process them async, is there any state that we build async
> related to spell checking or are you saying just the events are async? From
> what I can see, the text attributes code interrogates the DOM selection
> directly; there's no caching layer or anything like that. Am I perhaps
> missing something?

right, we receive DOM selection change events and transform those into a11y events. We don't always make the processing sync, in case if there are some pending changes on the stack, then the events will be fired async. So, it has to be a reason why onSpellCheck doesn't work for us.

> > Should we instead fire
> > text_attr change events
> 
> What's odd is that we don't seem to fire text atr change events in this case.

aha, that's a problem then. according to https://dxr.mozilla.org/mozilla-central/source/accessible/base/SelectionManager.cpp#225 we should fire the events. I'd say it makes sense to fix the problem, possibly in a separate bug. Besides that it will allow us to create an intermittent-proof test.

what is the case btw? @contentEditable="true"? so it works fine if editable div is replaced by HTML:input?

> > or at least it should be better to make an infinite
> > loop - the test will either hang if we have a bug, or it will finish
> > occasionally, but no intermittents.
> 
> I would have thought hanging is just a special case of an intermittent. :)

let me disagree :) It can fail only if we don't fire events. Otherwise it has to pass, right?

> Still, worst case scenario, maybe I can poll the first attribute range until
> it changes or something like that. Still ugly, but at least it'll be
> reliable. I'd really prefer a better solution, though.

right, I'd say figure out why we don't fire text change events is the best one.

Comment 6

[Drew Willcoxon :adw]

IIRC I used onSpellCheck when converting a bunch of tests that could previously rely on spell check's being sync.  It might be just good enough to have made those tests pass but not good enough for your case, especially with how it waits an arbitrary number of event loop turns before deciding that another spell check won't happen.

If I were you I would add some logging to the spell check code and your code to see if you can tell what exactly is happening.  It's hard to say what the fix should be without understanding the flow of events.

inlineSpellChecker-spellCheck-ended is broadcast here: https://dxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#831

It may be the case that we're incorrectly calling ChangeNumPendingSpellChecks early, before spell check finishes, but I would doubt it.  Or it may be the case that you're actually seeing multiple spell checks happening around the same time.

Comment 7

[James Teh [:Jamie]]

I've made a little progress on this. It seems onSpellCheck is in fact behaving as it should, at least as far as editor is concerned. Just before onSpellCheck calls the callback, if I ask the editor to stringify the spell check selection:
editor.selectionController.getSelection(Ci.nsISelectionController.SELECTION_SPELLCHECK).toString()
it reports "tsettset", which is correct; that's the two misspellings of "tset". So, the question is why a11y is still reporting no spelling error at that point.

(In reply to alexander :surkov (:asurkov) from comment #5)
> right, but why 40? Is it kinda special number or? :)

Only special in that I was experiencing intermittent failures locally with anything less than 30, so I sprung for 40.

> I'd say it makes sense
> to fix the problem[ of not firing text attr change events], possibly in a separate bug.

What I don't understand is why the events should matter here. HyperTextAccessible::TextAttributes calls HyperTextAccessible::GetSpellTextAttr, and that in turn calls GetFrameSelection() and then FrameSelection::GetSelection() to get the spell check selection. So, there doesn't seem to be any state that gets set up by the events or anything like that, nor is there any caching that needs to be invalidated. It should be just operating on the DOM selection... yet it doesn't work for quite some time. Is there some state I'm missing here?

Comment 8

[alexander :surkov (:asurkov)]

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

> > I'd say it makes sense
> > to fix the problem[ of not firing text attr change events], possibly in a separate bug.
> 
> What I don't understand is why the events should matter here.
> HyperTextAccessible::TextAttributes calls
> HyperTextAccessible::GetSpellTextAttr, and that in turn calls
> GetFrameSelection() and then FrameSelection::GetSelection() to get the spell
> check selection. So, there doesn't seem to be any state that gets set up by
> the events or anything like that, nor is there any caching that needs to be
> invalidated. It should be just operating on the DOM selection... yet it
> doesn't work for quite some time. Is there some state I'm missing here?

you have a point. If DOM selection reports spellcheck correctly, then it means a11y fails to transform those into text attributes. It could happen, when a11y have something on stack. For example, if contentEditable div gets initialized/changed on focus, and we rebuild the accessible tree. You might want to check this hypothesis by setting up a break point, but I'd say the text attribute change events is the only time when it's safe to assume text attribute were already changed. Note, this approach works equally well, if later we change architecture and will start caching or something.

Comment 9

[James Teh [:Jamie]]

I did a bit of debugging. It seems that in a11y, the selection does report two ranges, but we skip them for some reason. In the end, I gave up with that and decided to focus on figuring out why we weren't getting events, as you suggested. And...

I figured out what was going on with the text attr change events. The contentEditable contains two child divs. I was listening for the event on the first child div, thinking it would fire on both. In reality, it only fires on the second one. I guess that has something to do with the fact that we use the anchor/focus range and there are actually two ranges in the selection. There's a comment in the code about fixing this so it fires on the editor. For now, I've just commented the reason and I listen for the second div.

I still see cases where if I run with --run-until-failure, we sometimes don't get the event. However, it always works for at least the first time the test runs. This might bite us at some point, but if it does, as you say, that's a bug we should probably fix.

Comment 10

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #9)
> I still see cases where if I run with --run-until-failure, we sometimes
> don't get the event.

Setting spellcheck="true" from the beginning (rather than after focus) seems to fix this, and that also means we don't have to wait for the focus event. I don't really understand why, but that simplifies the test a lot anyway and it's exactly what will happen in most situations.

Comment 11

[alexander :surkov (:asurkov)]

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

> I figured out what was going on with the text attr change events. The
> contentEditable contains two child divs. I was listening for the event on
> the first child div, thinking it would fire on both. In reality, it only
> fires on the second one. I guess that has something to do with the fact that
> we use the anchor/focus range and there are actually two ranges in the
> selection. There's a comment in the code about fixing this so it fires on
> the editor. For now, I've just commented the reason and I listen for the
> second div.

got it

(In reply to James Teh [:Jamie] from comment #10)
> (In reply to James Teh [:Jamie] from comment #9)
> > I still see cases where if I run with --run-until-failure, we sometimes
> > don't get the event.
> 
> Setting spellcheck="true" from the beginning (rather than after focus) seems
> to fix this, and that also means we don't have to wait for the focus event.
> I don't really understand why, but that simplifies the test a lot anyway and
> it's exactly what will happen in most situations.

yeah, it's weird, but happy to hear it works :)

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 9001938 [details] [diff] [review]
Fix incorrect start offset when retrieving accessible text attributes if the last spelling error is not within this accessible.

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

cancelling request, it appears a new patch is coming

Comment 13

[James Teh [:Jamie]]

Attachment: Bug 1479678: Fix incorrect start offset when retrieving accessible text attributes if the last spelling error is not within this accessible.

When dealing with an editor which contains multiple accessibles, the previous spelling error range might be in a previous accessible, not the accessible currently being queried.
In this case, DOMPointToOffset will return the length of this accessible.
Previously, we weren't checking for this and were overriding the start offset of the returned range regardless, resulting in broken offsets.
Now, we leave the start offset alone in this case.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 9003050 [details]
Bug 1479678: Fix incorrect start offset when retrieving accessible text attributes if the last spelling error is not within this accessible.

alexander :surkov (:asurkov) has approved the revision.

Comment 15

[James Teh [:Jamie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4417ae20759620cca900656fca790d407a44f733

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c1a7b85ecbe
Fix incorrect start offset when retrieving accessible text attributes if the last spelling error is not within this accessible. r=surkov

Comment 17

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/2c1a7b85ecbe