Closed Bug 1827557 Opened 1 year ago Closed 1 year ago

[CTW][Mac][New Text Impl] Regressions in text navigation announcements

Categories

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

Desktop
macOS
defect

Tracking

()

VERIFIED FIXED
114 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox112 --- disabled
firefox113 + verified
firefox114 + verified

People

(Reporter: MarcoZ, Assigned: Jamie)

References

Details

(Keywords: regression, Whiteboard: [ctw-m8])

Attachments

(2 files)

In both single-line and multi-line text fields, there are some related regressions since the new text implementation for MacOS.

Steps to reproduce:

  1. Open data:text/html,<button>Before</button><textarea></textarea>
  2. Tab to the text field and enter some text. Make sure to get several lines of text out of it so you have room to maneuver.
  3. With VoiceOver turned on, read line by line by navigating up and down with arrow keys.
    • Expected: Every line should be read on its own.
    • Actual: The last line is only read if the cursor doesn't land at the end of the line, past the last character. if it lands there, the top line of the text field is read instead.
  4. Now position the cursor at the end of the line and navigate left and right by character.
    • Expected: Each character the cursor passes over should be read. And once past the last character, that should also be indicated. use Safari for comparison.
    • Actual: As soon as the last character should be read, the first character of the first line of the text field is being read. The last entered character is never voiced.
  5. Now navigate left and right by word, by using Option+Left and Right arrow keys.
    • Expected: All words should be read as the cursor passes over them.
    • Actual: Every word is read except the last one. When the last word should be read, the very first word of the very first line of the text field is read instead. The last entered word in the text field is never spoken.

I am filing this as a single bug because the results of the three scenarios is very similar: When the last line should be read, the very first is read instead, if the last character should be read, the very first character is read instead, and if the last word should be read, the very first word is read instead.

The character and word scenarios can also be reproduced in single-line text fields, AKA input type="text".

I suggest that this block the M8 milestone, since this is pretty severe and prevents proper text entry and proof reading. Feel free to change the taging/prioritization if you disagree.

This bug has been marked as a regression. Setting status flag for Nightly to affected.

Jamie, sounds like this could impact our decision to ship CtW by default on Mac for Fx113?

Yeah, this does seem pretty severe. :(

Severity: S3 → S2
Flags: needinfo?(jteh)
Priority: -- → P1
Assignee: nobody → jteh

CachedTextMarker uses TextLeafPoint.
Because caret and selection events currently use HyperText offsets, we construct text markers using a HyperTextAccessible.
The constructor detects that it was provided with a HyperText and converts to a TextLeafPoint appropriately.
However, this was previously conditional on the provided offset being less than the character count.
When the caret is at the insertion point at the end of a text box, the caret offset will be the character count itself.
This meant that we didn't convert to TextLeafPoint in this case, resulting in an incorrect text marker.
This was causing VoiceOver to report the first character, word, etc. when cursoring through text boxes instead of the last.

I can confirm that this patch partially fixes the bug. Characters and words now read as expected. Lines, however, are a different matter still.
If I position the caret at the very first character, and then move down, the line I just left is read instead of the line the cursor lands on. If I type a second paragraph and arrow up and down, I always get the line read that is the one above the caret. When the caret moves to a different line, that one should be read instead of the line I just left behind, or which is above the current one. When reading by line, the Mac doesn't follow the same rule as for characters and words, where the characters or words are being read that the caret just passed over. With lines, in other apps, the line is being read that the caret lands on, not the one it just moved away from.

Urgh. I've tracked the line bug down to CachedTextMarker::LeftLineRange not including the origin when fetching the start point. Making it include the origin fixes the bug. However, it also breaks a bunch of tests. Restricting it so it only does this if not at the start of a paragraph fixes some of those failures, but there are still unfixed failures. These tests pass with the cache disabled but lines aren't broken with VO with the cache disabled, so clearly, there's some edge case here I'm missing.

Attachment #9330346 - Attachment description: Bug 1827557: When constructing a CachedTextMarker from a HyperText, allow an offset equal to the character count. → Bug 1827557 part 1: When constructing a CachedTextMarker from a HyperText, allow an offset equal to the character count.

Otherwise, VoiceOver reports the previous line if you're on the first character of a line and you move by line with down or up arrow.

See Also: → 1830208
Attachment #9330520 - Attachment description: Bug 1827557 part 2 WIP: CachedTextMarker::LeftLineRange: Return the current line (not the previous one) when at the start of a line. → Bug 1827557 part 2 WIP: CachedTextMarker: Differentiate between LeftLine and Line.
Attachment #9330520 - Attachment description: Bug 1827557 part 2 WIP: CachedTextMarker: Differentiate between LeftLine and Line. → Bug 1827557 part 2: CachedTextMarker: Differentiate between LeftLine and Line.
Flags: needinfo?(marco.zehe)

I am running with this build now, and am happy to report that lines are read correctly with VoiceOver. As I type this comment, and lines wrap, VoiceOver correctly tracks the new line when I do a SayLine with VO+L.

Also when I add a new paragraph and then navigate, everything is tracked properly.
There is one edge I found. If I insert two line breaks, like above, and then navigate to the blank line, VoiceOver does read the below line instead of saying that this is a blank line. But if I leave a blank line at the very end of the text area, it is correctly read as a blank line. So the only edge case is two new lines, the caret moves between them, and VoiceOver reads the next line instead.
Interestingly, I have seen this particular scenario fail in other browsers, too, on occasion, so this seems to be a hard one to get right. If you can't figure this one out immediately, I think it would be totally reasonable to file it as a separate bug, check these two patches in and uplift them so CTW for Mac can make the 113 cut after all.

Flags: needinfo?(marco.zehe)

Thanks. I'll look into it with priority, but yeah, I'll file it as a separate bug. That seems far less severe than this one and I don't think it needs to block ship.

Is the blank line issue definitely a regression from CtW? If you're not sure, no problem; I'll check myself.

I am definitely not sure if that one is a regression from before CTW, or even the first TextMarker implementation. Sorry! But like I said, this shouldn't block ship.

Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67f4197873b6
part 1: When constructing a CachedTextMarker from a HyperText, allow an offset equal to the character count. r=morgan
https://hg.mozilla.org/integration/autoland/rev/37817b0799e3
part 2: CachedTextMarker: Differentiate between LeftLine and Line. r=eeejay

Comment on attachment 9330346 [details]
Bug 1827557 part 1: When constructing a CachedTextMarker from a HyperText, allow an offset equal to the character count.

Beta/Release Uplift Approval Request

  • User impact if declined: Incorrect reporting with VoiceOver screen reader when navigating text boxes.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Deals with specific edge cases in Mac a11y text reporting. Covered by automated tests.
  • String changes made/needed:
  • Is Android affected?: No
Attachment #9330346 - Flags: approval-mozilla-beta?
Attachment #9330520 - Flags: approval-mozilla-beta?

Comment on attachment 9330346 [details]
Bug 1827557 part 1: When constructing a CachedTextMarker from a HyperText, allow an offset equal to the character count.

This hasn't merged to m-c yet, but the CI results on autoland are looking good. Let's get this landed for 113.0b9 so we can get as much testing as possible over the weekend and make an informed go/no-go decision for CtW on Mac before we build the RC on Monday.

Attachment #9330346 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

I'd feel better if QA could do some exploratory testing and sanity check with VoiceOver on 113.0b9 if at all possible.

Flags: qe-verify+
Attachment #9330520 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 114 Branch

I can confirm that this is fixed in Firefox 114.0a1 ( 20230427213157). Reading by character, word, and line now work as expected, minus the edge case where with two consecutive line breaks, on the blank line, the next line is read instead. @jamie have you filed a bug on this already, or should I?

Status: RESOLVED → VERIFIED
Flags: needinfo?(jteh)

I can also confirm that the patches landed properly in Firefox 113.0b9. In fact, I am using it right now to compose this comment.
I'll leave it to the official QE team to do the final verification and mark-up. But I am confident that my results will be verified. Great job, everyone!

If you wouldn't mind filing a bug about the other issue, that'd be great. I can do it of course if you don't have time, but I'm less familiar with VoiceOver on the Mac than you are, so it'd be good to ensure the details are as correct as possible. Thanks for all your help.

Flags: needinfo?(jteh)

Done, filed bug 1830466 and tentatively made it block the ctw-text meta bug. I'll leave the prioritization and other sorting up to you.

Blocks: 1830466
QA Whiteboard: [qa-triaged]

This issue is Verified as fixed in our latest Beta and Nightly builds, We also did a bit of exploratory testing on this and we were able to reproduce the bug mentioned in Comment 23, the downside to this is that when there are 2 empty lines and a third one with text, the Third line will be read twice, once when the user reaches the 2nd empty line and then when it reaches the line with the text.

Safari says "New line" each time it reaches an Empty line.

QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: