TextLeafPoint: Implement support for spelling errors
Categories
(Core :: Disability Access APIs, task)
Tracking
()
Tracking | Status | |
---|---|---|
firefox102 | --- | fixed |
People
(Reporter: Jamie, Assigned: Jamie)
References
Details
(Whiteboard: [ctw-m2])
Attachments
(5 files)
Bug 1730096 implements TextLeafPoint support for all text attributes except spelling errors. Spelling errors (and eventually grammar errors) are more complicated because they can span just part of a text leaf. The same will be true for the upcoming CSS Custom Highlight API.
For spelling errors, I'm thinking it probably makes sense to cache the spelling error selection ranges on the document. We're probably going to have to do similarly for normal text selection. When searching for the start of a text attribute run, we'd then tweak the returned range if it overlapped any spelling error range.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
Some notes on my investigation:
An alternative to caching the spelling error selection ranges is to cache an array of spelling error offsets for each TextLeafAccessible. We could get this info using dom::Selection::GetRangesForIntervalArray. The advantage is that this would be easier to handle on the parent process side. However, I realised this isn't feasible because when the spelling error selection changes, we'd need to send cache updates for the currently covered nodes as well as the previously covered nodes, since some errors might have been removed. The problem is that we don't know what the previous ranges were.
So, I think we'll just need to send all the ranges whenever the spelling error selection changes. This could get a little noisy, but I guess we'll just have to see if it ends up being a real problem. If it is, we'll need to find some way to get notified about the specific errors that were added/removed.
Assignee | ||
Comment 2•3 years ago
|
||
(In reply to James Teh [:Jamie] from comment #1)
when the spelling error selection changes, we'd need to send cache updates for the currently covered nodes as well as the previously covered nodes, since some errors might have been removed.
While nsISelectionListener::NotifySelection doesn't specifically tell us what ranges changed, we could probably add some a11y notifications in dom::Selection::Add/RemoveRangeAndSelectFramesAndNotifyListeners, since that does know the specific range which changed.
Doing this per-node would still be a bit awkward, since we probably want to iterate the ranges once and cache the data on all the impacted leaf nodes rather than iterating all the ranges for every leaf node. That means this won't fit the usual BundleFieldsForCache paradigm.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 3•3 years ago
•
|
||
I implemented this fully by storing the ranges on the document/text fields. This works... until you insert or remove text from a node within one of the ranges without a spelling error changing as a result. This causes the offsets in the range to become invalid, but because no spelling errors changed, there's no cache update for them. So, this approach can't work. We need to update the spelling error ranges for a specific leaf node when the text of that node changes, which means we have to cache spelling errors per leaf node.
(In reply to James Teh [:Jamie] from comment #2)
Doing this per-node would still be a bit awkward, since we probably want to iterate the ranges once and cache the data on all the impacted leaf nodes
We can't do this for the same reason as above. This has to be done per leaf node.
Assignee | ||
Comment 4•3 years ago
|
||
This was done for LocalAccessibles to avoid the need to fetch attributes twice when we're simultaneously fetching boundaries and returning the attributes to a client.
However, GetTextAttributes will soon handle spelling errors, so it will no longer return the same data as GetTextAttributesLocalAcc (which doesn't handle spelling errors).
This breaks the equality checks in FindTextAttrsStart.
Aside from this, this argument was somewhat confusing.
For now, just remove aOriginAttrs, which means there will be some redundant calls to GetTextAttributesLocalAcc.
Parent process documents shouldn't be that large anyway.
If this ends up being a real problem, we can either revert to the local HyperTextAccessible implementation of text attributes or implement a smarter temporary cache.
Assignee | ||
Comment 5•3 years ago
|
||
The implementation searches for spelling errors in each leaf, as this is how we will need to do it for RemoteAccessibles.
Assignee | ||
Comment 6•3 years ago
|
||
Even though spelling errors can cross Accessibles and are represented in the DOM as ranges, we cache them for each text leaf.
This is necessary so that we correctly update the offsets when text is inserted or removed from a leaf.
We cache them as an array of offsets, including both the start and end offset for each range.
We use -1 as the start offset to indicate when a spelling error starts in a previous Accessible.
When a spelling error starts in an Accessible but ends in a subsequent one, we simply don't include an end offset in the array.
This structure means we can easily query spelling error points or ranges with a binary search.
Assignee | ||
Comment 7•3 years ago
|
||
We already have an nsISelectionListener, but that only tells us that a change happened somewhere in the selection, not which range changed.
We don't want to push a cache update for all ranges when only one changed.
Therefore, this patch adds an accessibility notification in dom::Selection::AddRangeAndSelectFramesAndNotifyListeners.
We don't need this for removed ranges because the text will change for any spelling error corrections and text updates trigger spelling error cache updates.
Assignee | ||
Comment 8•3 years ago
|
||
Comment 10•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/16aee29be1b7
https://hg.mozilla.org/mozilla-central/rev/08d033b9651b
https://hg.mozilla.org/mozilla-central/rev/388a15605114
https://hg.mozilla.org/mozilla-central/rev/69d2ec7b4d6c
https://hg.mozilla.org/mozilla-central/rev/c50022d7bfdf
Description
•