Skip to content

AX: When navigating by-line with VoiceOver, sometimes lines are double-read and others are skipped #47265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 27, 2025
Merged

AX: When navigating by-line with VoiceOver, sometimes lines are double-read and others are skipped #47265

merged 1 commit into from
Jun 27, 2025

Conversation

twilco
Copy link
Contributor

@twilco twilco commented Jun 26, 2025 •
edited by webkit-early-warning-system
Loading

d059afd

AX: When navigating by-line with VoiceOver, sometimes lines are double-read and others are skipped
https://bugs.webkit.org/show_bug.cgi?id=295060
rdar://154432630

Reviewed by Joshua Hoffman.

This happened because when moving up and down lines via character navigation, we would end up at a text position at the
very end of a line (but still within bounds of the text element), then post this position as a text marker to VoiceOver.
This resulted in us computing the same line-range twice for two different markers across two different up / down movements,
resulting in one line being read twice and the next one being skipped.

The main-thread implementation avoids this using RenderedPosition, which was built to avoid these ambiguities when it comes
to DOM text vs. actual rendered text (e.g. post whitespace collapsing and trimming). RenderedPosition is in turn constructed
using VisiblePosition::inlineBoxAndOffset, which is what has the smarts on when to move off of these ambiguous positions.

This commit fixes the bug by changing AXObjectCache::textMarkerDataForVisiblePosition to lean on VisiblePosition::inlineBoxAndOffset
when constructing an AX-thread text marker. New layout test added (accessibility/mac/line-navigation-skipped-content.html)
that fails without this change.

This commit also includes a fix in AXTextRuns::runEndsWithLineBreak to address a regression introduced by https://commits.webkit.org/296681@main.
When initially working on that change, AXTextRun::endIndex was inclusive, but I later made it exclusive, and forgot to
update runEndsWithLineBreak appropriately (with a `- 1`).

* LayoutTests/accessibility/mac/line-navigation-skipped-content-expected.txt: Added.
* LayoutTests/accessibility/mac/line-navigation-skipped-content.html: Added.
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::textMarkerDataForVisiblePosition):
* Source/WebCore/accessibility/AXTextRun.h:
(WebCore::AXTextRuns::runEndsWithLineBreak const):

Canonical link: https://commits.webkit.org/296702@main

bd0a55c

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@twilco twilco self-assigned this Jun 26, 2025
@twilco twilco added the Accessibility For bugs related to accessibility. label Jun 26, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 26, 2025 •
edited
Loading

EWS run on current version of this PR (hash bd0a55c)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

minorninth
minorninth reviewed Jun 27, 2025
View reviewed changes
Source/WebCore/accessibility/AXTextRun.h
@@ -179,7 +179,7 @@ struct AXTextRuns {

// Convenience methods for TextUnit movement.
bool runStartsWithLineBreak(size_t runIndex) const { return text[runs[runIndex].startIndex] == '\n'; }
bool runEndsWithLineBreak(size_t runIndex) const { return text[runs[runIndex].endIndex] == '\n'; }
bool runEndsWithLineBreak(size_t runIndex) const { return text[runs[runIndex].endIndex - 1] == '\n'; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this should have a bounds check, because isn't it possible endIndex is 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This RELEASE_ASSERT in the AXTextRun constructor guarantees endIndex will never be zero. WTF::Vector also inherently does a bounds check on every access.

minorninth
minorninth approved these changes Jun 27, 2025
View reviewed changes
Copy link
Contributor

@minorninth minorninth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix!

@hoffmanjoshua
Copy link
Contributor

LGTM!

@twilco twilco added the merge-queue Applied to send a pull request to merge-queue label Jun 27, 2025
@twilco
AX: When navigating by-line with VoiceOver, sometimes lines are doubl…
d059afd 
…e-read and others are skipped

https://bugs.webkit.org/show_bug.cgi?id=295060
rdar://154432630

Reviewed by Joshua Hoffman.

This happened because when moving up and down lines via character navigation, we would end up at a text position at the
very end of a line (but still within bounds of the text element), then post this position as a text marker to VoiceOver.
This resulted in us computing the same line-range twice for two different markers across two different up / down movements,
resulting in one line being read twice and the next one being skipped.

The main-thread implementation avoids this using RenderedPosition, which was built to avoid these ambiguities when it comes
to DOM text vs. actual rendered text (e.g. post whitespace collapsing and trimming). RenderedPosition is in turn constructed
using VisiblePosition::inlineBoxAndOffset, which is what has the smarts on when to move off of these ambiguous positions.

This commit fixes the bug by changing AXObjectCache::textMarkerDataForVisiblePosition to lean on VisiblePosition::inlineBoxAndOffset
when constructing an AX-thread text marker. New layout test added (accessibility/mac/line-navigation-skipped-content.html)
that fails without this change.

This commit also includes a fix in AXTextRuns::runEndsWithLineBreak to address a regression introduced by https://commits.webkit.org/296681@main.
When initially working on that change, AXTextRun::endIndex was inclusive, but I later made it exclusive, and forgot to
update runEndsWithLineBreak appropriately (with a `- 1`).

* LayoutTests/accessibility/mac/line-navigation-skipped-content-expected.txt: Added.
* LayoutTests/accessibility/mac/line-navigation-skipped-content.html: Added.
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::textMarkerDataForVisiblePosition):
* Source/WebCore/accessibility/AXTextRun.h:
(WebCore::AXTextRuns::runEndsWithLineBreak const):

Canonical link: https://commits.webkit.org/296702@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/AX-When-navigating-by-line-with-VoiceOver-sometimes-lines-are-double-read-and-others-are-skipped branch from bd0a55c to d059afd Compare June 27, 2025 03:15
@webkit-commit-queue
Copy link
Collaborator

Committed 296702@main (d059afd): https://commits.webkit.org/296702@main

Reviewed commits have been landed. Closing PR #47265 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit d059afd into WebKit:main Jun 27, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minorninth minorninth minorninth approved these changes

@hoffmanjoshua hoffmanjoshua hoffmanjoshua approved these changes

Assignees

@twilco twilco

Labels
Accessibility For bugs related to accessibility.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@twilco @webkit-early-warning-system @hoffmanjoshua @webkit-commit-queue @minorninth