1909142 - Expose CSS custom highlights to a11y APIs

Status: RESOLVED FIXED

(Core :: Disability Access APIs, task)

Description

[James Teh [:Jamie]]

The CSS custom highlight API allows pages to highlight arbitrary ranges of text. Optionally, the highlight can be marked as a spelling or grammar error. These highlights need to be exposed to accessibility APIs; e.g. so that screen readers can report them.

The foundational work in bug 1905211 should make this relatively straightforward. However, custom highlights do allow for overlapping ranges and I'm not entirely sure the a11y code handles that correctly.

Broad implementation steps:

1. Add a way to retrieve highlight selections to nsFrameSelection. Because there can be an arbitrary number of highlight selections, nsFrameSelection::GetSelection can't be used to retrieve these.
2. Extend FindDOMTextOffsetAttributes in accessible/base/TextLeafRange.cpp to handle highlight selections. We can't include these in kSelectionTypesToAttributes because of their arbitrary number, so we'll likely need to move the body of the loop into a lambda or similar and call it for both static selection types and highlight selections.
3. Extend TextLeafPoint::AddTextOffsetAttributes to expose the correct attributes for custom highlights.
4. Check whether overlapping ranges are handled correctly. If they aren't, fix it in TextLeafPoint::AddTextOffsetAttributes and/or TextLeafPoint::FindTextOffsetAttributeSameAcc.

Comment 1

[James Teh [:Jamie]]

Hi Jan. Would you be able to answer some questions about overlapping custom highlight ranges?

1. As I understand it, custom highlight ranges can overlap. Does this mean that there can be overlapping ranges within a single Highlight object (and thus a single dom::Selection), overlapping ranges in different Highlight objects (and thus different dom::Selections), or both?
2. This comment in the DOM code suggests that "overlapping ranges are disallowed". This was done all the way back in bug 348681. Is that no longer true and this comment is just out of date? Or is there something I'm missing here?
3. As I understand it, ranges are sorted within a dom::Selection and GetAbstractRangesForIntervalArray returns a sorted array of ranges. How then are overlapping ranges handled? Are they sorted by start offset only? Or are they sorted by end offset? Or something else?

Anything else you think might be relevant regarding overlapping ranges would also be super helpful.

Thanks!

Comment 2

[James Teh [:Jamie]]

Some digging in the code suggests that ranges are sorted only by start offset. What confuses me though is how overlapping ranges are correctly handled by GetIndicesForInterval, which discards "Ranges that end before the given interval" using a binary search. But if there are overlapping ranges, the ends won't be sorted. For example:
(0, 9), (1, 2), (2, 3)
That would surely break a binary search. I feel like I must be missing something here, but I can't work out what.

Comment 3

[Jan Jaeschke [:jjaschke]]

Re. comment 0:

1. Highlight selections are stored in nsFrameSelection::mHighlightSelections. A const getter should be straightforward.

Re. comment 1:

1. When ranges of one highlight overlap, there is no visual indication -- it looks as if there is only one range. However, for Selection, there are multiple. Source in the spec. Several ranges of different highlights can also overlap. In that case, the priority of the highlight decides which one is visually drawn. If there are two highlights which have the same priority, the one which is created later (ie., registered in the highlight registry later) takes precedence. Note that the colors are not combined (e.g. if there were opacity in the colors). Source in the spec.
2. Yeah, it seems that comment is outdated. Overlapping ranges are allowed only if type == Highlight. That's a consequence of the design decision I made back then to squeeze Custom Highlight into Selection. As to comment 2, there is a chance that there are bugs in that.

I'm wondering, do we actually need to consider the overlapping ranges for a11y, or would it also work if they are merged into one? (I absolutely have not thought this through)

Comment 4

[James Teh [:Jamie]]

Merging them might help. Are you suggesting a11y would need to merge these though or that dom::Selection might do this? Merging them in a11y isn't something we'd want to do repeatedly, but we don't have a cache for parent process documents.

That said, a11y currently puts all of the highlight ranges (across multiple dom::Selections) into a single array. This means we run into problems when ranges from different Highlights overlap, not just overlapping ranges in the same Highlight. I think I'm going to need to re-think that unfortunately, since I doubt DOM could do any merging there even if it did so within a single dom::Selection.

Comment 5

[Jan Jaeschke [:jjaschke]]

Merging them might help. Are you suggesting a11y would need to merge these though or that dom::Selection might do this? Merging them in a11y isn't something we'd want to do repeatedly, but we don't have a cache for parent process documents.

I'm wondering if merging the ranges would also make painting simpler (read: faster), I had to implement some additional logic if I remember correctly.
The code for merging the ranges already exists in dom::Selection, since merging is the standard behavior for non-highlight selections. So, for highlight selections we could introduce a new ranges array, so that there is one range array for the merged ranges, which is used by a11y and sent to painting, and one which contains the unmerged ranges and is used for interacting with JS...

That said, a11y currently puts all of the highlight ranges (across multiple dom::Selections) into a single array. This means we run into problems when ranges from different Highlights overlap, not just overlapping ranges in the same Highlight. I think I'm going to need to re-think that unfortunately, since I doubt DOM could do any merging there even if it did so within a single dom::Selection.

That sounds tricky. I guess in addition to overlapping the order of the overlapped ranges needs to be taken into consideration.
I don't know if it helps and the situation can be mapped to a11y, but this is roughly how this situation is handled in painting: content is split every time a range starts or ends, and each of these "split content sub ranges" has a list of associated ranges, ordered by their priority.

Comment 6

[James Teh [:Jamie]]

Attachment: Bug 1909142 part 1: Add Selection and nsFrameSelection getters to enable the accessibility engine to query custom highlight information.

Comment 7

[James Teh [:Jamie]]

Attachment: Bug 1909142 part 2: Expose custom highlights via accessibility APIs.

Comment 8

[James Teh [:Jamie]]

Attachment: Bug 1909142 part 3: Fix accessibility handling of overlapping custom highlight ranges.

Before custom highlights, ranges from a dom::Selection couldn't overlap and thus TextLeafRange didn't handle this.
However, custom highlight ranges can overlap.

Because we can't sort by both start and end offset any more, we can unfortunately no longer binary search the array of ranges for RemoteAccessible.
Hopefully, there won't be so many text offset attribute ranges in a single text leaf that this will be problematic.
Gecko's own spell checker stops at 500 misspelled words in a text node.
If it does cause problems, we could split the ranges into smaller, non-overlapping ranges when we send them from the content process.

Note that overlapping spelling and grammar errors aren't yet supported due to their use of the same "invalid" text attribute, but I expect this should be rare and can be fixed in future if needed.
However, this does handle highlights overlapping with either spelling or grammar errors.

Comment 9

[James Teh [:Jamie]]

(In reply to Jan Jaeschke [:jjaschke] from comment #5)

we could introduce a new ranges array, so that there is one range array for the merged ranges, which is used by a11y

FWIW, after working on this a bit more, I don't think this will really help a11y much because we ideally want to cache the highlight ranges in a single array, rather than having 1 array per highlight. That might change if we hit performance problems though. I'll keep you posted.

I guess in addition to overlapping the order of the overlapped ranges needs to be taken into consideration.

Because a11y can only expose all highlights the same way, regardless of their styling, I don't think the priority matters for a11y. We can expose the different types if highlights of different types overlap (e.g. a highlight and a spelling-error), but if there are 2 of the same type, a11y clients can't perceive any difference; the APIs for that just don't exist. That might change one day, but for now, a11y ignores the priority.

Comment 10

[James Teh [:Jamie]]

Since we can't binary search overlapping ranges, I ended up going with a linear walk, which hopefully won't be too slow. Note that the ranges are only per node; we're not walking the ranges for the entire document or anything like that. If it turns out to be a problem, one alternative could be to split the overlapping ranges into multiple smaller ranges that never overlap in TextLeafPoint::GetTextOffsetAttributes. This doesn't help LocalAccessible, but this is less likely to impact LocalAccessible anyway and it was always a linear walk in the first place to some extent.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ca7e6d2bcab
part 1: Add Selection and nsFrameSelection getters to enable the accessibility engine to query custom highlight information. r=jjaschke
https://hg.mozilla.org/integration/autoland/rev/91b09cf9d29b
part 2: Expose custom highlights via accessibility APIs. r=morgan
https://hg.mozilla.org/integration/autoland/rev/86c70282c41a
part 3: Fix accessibility handling of overlapping custom highlight ranges. r=morgan

Comment 12

[amarc]

Backed out for causing build bustages @ Selection.h

• Backout link
• Push with failures
• Failure Log
• Failure line:

/builds/worker/workspace/obj-build/dist/include/mozilla/dom/Selection.h:609:3: error: declaration of 'const mozilla::dom::HighlightSelectionData& mozilla::dom::Selection::HighlightSelectionData() const' [-fpermissive]

Mochitest failure: https://treeherder.mozilla.org/logviewer?job_id=493540598&repo=autoland&lineNumber=22813

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4499635e85e1
part 1: Add Selection and nsFrameSelection getters to enable the accessibility engine to query custom highlight information. r=jjaschke

Comment 14

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/4499635e85e1

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbdf17d42860
part 2: Expose custom highlights via accessibility APIs. r=morgan
https://hg.mozilla.org/integration/autoland/rev/be5fc8b8abd5
part 3: Fix accessibility handling of overlapping custom highlight ranges. r=morgan

Comment 16

[Sandor Molnar[:smolnar]]

Backed out for causing bc failures @ browser_text.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/0bc2a4516e6b56dc55099aff6d6c3e9ace489d35

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | accessible/tests/browser/text/browser_text.js

Buffered messages finished
[task 2025-02-07T07:20:29.296Z] 07:20:29     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/text/browser_text.js | waiting for vsync to be disabled - timed out after 50 tries. - false == true - 
[task 2025-02-07T07:20:29.297Z] 07:20:29     INFO - Stack trace:
[task 2025-02-07T07:20:29.297Z] 07:20:29     INFO - chrome://mochikit/content/browser-test.js:ensureVsyncDisabled:607
[task 2025-02-07T07:20:29.298Z] 07:20:29     INFO - chrome://mochikit/content/browser-test.js:nextTest:782
[task 2025-02-07T07:20:29.298Z] 07:20:29     INFO - chrome://mochikit/content/browser-test.js:testScope/test_finish/<:1813
[task 2025-02-07T07:20:29.298Z] 07:20:29     INFO - chrome://mochikit/content/browser-test.js:run:1733
[task 2025-02-07T07:20:29.299Z] 07:20:29     INFO - Not taking screenshot here: see the one that was previously logged
[task 2025-02-07T07:20:29.300Z] 07:20:29     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/text/browser_text.js | vsync remained enabled at the end of the test. Is there an animation still running? Consider talking to the performance team for tips to solve this. - false == true - 
[task 2025-02-07T07:20:29.300Z] 07:20:29     INFO - Stack trace:
[task 2025-02-07T07:20:29.300Z] 07:20:29     INFO - chrome://mochikit/content/browser-test.js:ensureVsyncDisabled:608
[task 2025-02-07T07:20:29.300Z] 07:20:29     INFO - chrome://mochikit/content/browser-test.js:nextTest:782
[task 2025-02-07T07:20:29.300Z] 07:20:29     INFO - chrome://mochikit/content/browser-test.js:testScope/test_finish/<:1813
[task 2025-02-07T07:20:29.301Z] 07:20:29     INFO - chrome://mochikit/content/browser-test.js:run:1733
[task 2025-02-07T07:20:29.301Z] 07:20:29     INFO - GECKO(7119) | MEMORY STAT | vsize 20984701MB | residentFast 1825MB
[task 2025-02-07T07:20:29.302Z] 07:20:29     INFO - TEST-OK | accessible/tests/browser/text/browser_text.js | took 24704ms

Comment 17

[James Teh [:Jamie]]

I fixed a bug here and have a try run showing the previously failing job now passing. However, it was very difficult to tell precisely what was causing this failure, so it might strike again. I've landed it again with the fix and I guess we'll see what happens. 🤞

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7e848418267
part 2: Expose custom highlights via accessibility APIs. r=morgan
https://hg.mozilla.org/integration/autoland/rev/188f02180645
part 3: Fix accessibility handling of overlapping custom highlight ranges. r=morgan

Comment 19

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/b7e848418267
https://hg.mozilla.org/mozilla-central/rev/188f02180645