[a11y] Only request text cursor on screen for caret moves [chromium/src : main]
Aaron Moss (Gerrit)
Message from Aaron Moss![]( "Open in Gerrit")
Set Ready For Review
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Moss (Gerrit)
Aaron Moss added 1 comment![]( "Open in Gerrit")
Here's my re-land of Magnification-follows-text-cursor, I thought I'd run the changes by you two before asking for owner review.
I have successfully run it through CL Dry-Run and some quick tests on my dev phone -- In both Chromium and WebView-in-Gmail, this does implement the desired Magnification-follows-Cursor behaviour, but does not re-introduce the "caret locks scrolling" bug.
One thing I wasn't quite clear on consensus on from our email thread was whether I should be doing Finch *and* per-embedder flags, or only one. I figured having a Finch experiment gave enough of a safety net for the wider Android version rollout, and that if I got my Finch config correct it should only roll out for Chrome and WebView, but glad to change any of those details if either of you have feedback.
Related details
- Bo Liu
- Ziad Youssef
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
Bo Liu added 4 comments![]( "Open in Gerrit")
view.requestRectangleOnScreen(boundsInView, /* immediate= */ false);keep this empty
there should be no logic in this class except to call through to the new method (if it exists)
private Rect fromDipToScreenBounds(float left, float top, float right, float bottom) {uhh.. does it also account for split or floating windows?
if (delegate != null) {this isn't a check for 36.1 though, it's a check for if we are built with the Delegate impl class, and there's no way to distinguish between 36 and 36.1, so if you do want to do the check, you have to make it part of the delegate
containerView.requestRectangleOnScreen(caretScreen);this takes view space, not screen space, and I guess the 36.1 method is the same? in which case it answer the comment above
Related details
- Aaron Moss
- Ziad Youssef
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ziad Youssef (Gerrit)
Ziad Youssef added 1 comment![]( "Open in Gerrit")
Thanks Aaron, I am not really familiar with this code. I will defer to Bo.
Related details
- Aaron Moss
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Moss (Gerrit)
Aaron Moss added 4 comments![]( "Open in Gerrit")
view.requestRectangleOnScreen(boundsInView, /* immediate= */ false);keep this empty
there should be no logic in this class except to call through to the new method (if it exists)
Can do, I'll put the fallback code at the callsite only.
For what it's worth, I thought this would be a reasonable default behaviour because the only new behaviour added in API level 36.1 is to add a [new overload with a source parameter](https://developer.android.com/reference/android/view/View#requestRectangleOnScreen(android.graphics.Rect,%20boolean,%20int)), which allows the system to ignore some `requestRectangleOnScreen()` calls based on user preferences.
private Rect fromDipToScreenBounds(float left, float top, float right, float bottom) {uhh.. does it also account for split or floating windows?
As I understand it, these are view-relative coordinates in physical pixel dimensions, but I did double-check that this worked properly with split screen on my dev phone (both vertical and horizontal split), and clarified the documentation.
this isn't a check for 36.1 though, it's a check for if we are built with the Delegate impl class, and there's no way to distinguish between 36 and 36.1, so if you do want to do the check, you have to make it part of the delegate
The [companion internal CL](https://chrome-internal-review.googlesource.com/c/clank/internal/apps/+/8754236) checks `Build.VERSION.SDK_INT_FULL >= Build.VERSION_CODES_FULL.BAKLAVA_1` in the delegate.
It's slightly awkward right now in that the 36.1 API has been finalized and is in public beta (and is the Android SDK available for internal code), but I don't _think_ public Chromium rolls to that Android SDK until the stable release, at which point I'll follow up with crbug.com/450540343 to move that version check out of the delegate into public code.
this takes view space, not screen space, and I guess the 36.1 method is the same? in which case it answer the comment above
- Bo Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
Bo Liu voted Code-Review+1![]( "Open in Gerrit")
- Aaron Moss
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Moss (Gerrit)
Aaron Moss added 1 comment![]( "Open in Gerrit")
Hi, looking for owner review on this fix to move code from `updateFrameInfo()` (called on every rendered frame) to `updateCursorAnchorInfo()` (called on every caret move).
I did need to add the caret information to the [`InputCursorAnchorInfo`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/input/ime_host.mojom;drc=c192edeace2afd28cd0b1383c526ab8e4a13ae24;l=29) Mojo struct, but per the linked docs it summarizes [Android's `CursorAnchorInfo`](https://developer.android.com/reference/android/view/inputmethod/CursorAnchorInfo), which does include an "insertion marker" field not previously represented in the Mojo struct.
Related details
- Arthur Sonzogni
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
aleventhal, could you take a look at the blink change from an a11y point of view, please :)
Related details
- Aaron Leventhal
- Aaron Moss
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Hi, looking for owner review on this fix to move code from `updateFrameInfo()` (called on every rendered frame) to `updateCursorAnchorInfo()` (called on every caret move).
I did need to add the caret information to the [`InputCursorAnchorInfo`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/input/ime_host.mojom;drc=c192edeace2afd28cd0b1383c526ab8e4a13ae24;l=29) Mojo struct, but per the linked docs it summarizes [Android's `CursorAnchorInfo`](https://developer.android.com/reference/android/view/inputmethod/CursorAnchorInfo), which does include an "insertion marker" field not previously represented in the Mojo struct.
Assuming you found aleve...@google.com, I am removing myself. Feel free to reassign to me.
Aaron Moss (Gerrit)
Aaron Moss added 1 comment![]( "Open in Gerrit")
aleventhal, could you take a look at the blink change from an a11y point of view, please :)
One possibly relevant note here is that the Android Magnification service doesn't use the accessibility tree, and, as such, doesn't expose itself on a technical level as an AT, which is why this change is routed through the IME code rather than some more-explicitly accessibility path.
Related details
- Aaron Leventhal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Moss (Gerrit)
Aaron Moss added 1 comment![]( "Open in Gerrit")
Hi David, Nico wanted an Accessibility review on this PR (a re-land of my Magnification-follows-text-cursor), but tagged in Aaron Leventhal, who's no longer on the team; would you mind taking a look?
Related details
- Bo Liu
- David Tseng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Tseng (Gerrit)
David Tseng added 8 comments![]( "Open in Gerrit")
* @param boundsInView the rect to request on screen, in coordinates relative to {@code view}Good catch on the param mismatch. I would have expected some of this be caught be a linter?
* and content Y offset.Would there ever be a content x offset?
* @param bottmo bottom Y coordinate in CSS pixelsnit: bottom
// Request view system keep caret on screen when movedsupernit: add period at end.
AconfigFlaggedApiDelegate delegate =In AconfigFlaggedApiDelegate.java, they recommend calling AconfigFlaggedApiDelegate.getInstance. Did you try that before?
" * Prefer to use this to get a instance instead of calling ServiceLoaderUtil. If possible, avoid
* caching the return value in member or global variables as it allows more compile time"
(written regarding getInstance)
containerView.requestRectangleOnScreen(caretPix);It may be worth pointing out that this is the fallback. That is, the original request api that does not contain a focus source.
Was this path requested by the magnifier proposal?
gfx.mojom.Rect? insertion_marker;Should this be called caret_bounds?
insertion_marker_info = std::move(focus_rect);Are you sure `focus_rect` is the right choice here?
What is `bounding_box_rect`? What happens if anchor is first? Should we use `anchor_rect` then? If you have a good understanding of
GetSelectionBoundsInWindow
then that works for me, but we want to get this part nailed down.
Related details
- Aaron Moss
- Bo Liu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aaron Moss (Gerrit)
Aaron Moss added 8 comments![]( "Open in Gerrit")
* @param boundsInView the rect to request on screen, in coordinates relative to {@code view}Good catch on the param mismatch. I would have expected some of this be caught be a linter?
Yeah, who knows; I think there's a Javadoc linter that ships with the Java SDK, but it's wildly over-picky (it will insist on `@param` and and `@return` tags for every method, even when they diminish clarity), best guess without looking it up our Javadoc linters are a bit over-permissive on this case instead.
Would there ever be a content x offset?
I don't expect so.
With more context, there certainly isn't a getter for an X offset in the `RenderCoordinates` type. As best I understand it, one isn't necessary, because the Y-axis content offset is mostly (entirely?) there to support the address bar chrome, which (presumably because of some non-trivial animations and/or interaction with layout) is part of the same `View` as the WebView. Because these are view-local coordinates, any X (or Y) offset coming from other views is already accounted for.
* @param bottmo bottom Y coordinate in CSS pixelsAaron Mossnit: bottom
Fixed
// Request view system keep caret on screen when movedsupernit: add period at end.
Fixed
In AconfigFlaggedApiDelegate.java, they recommend calling AconfigFlaggedApiDelegate.getInstance. Did you try that before?
" * Prefer to use this to get a instance instead of calling ServiceLoaderUtil. If possible, avoid
* caching the return value in member or global variables as it allows more compile time"
(written regarding getInstance)
Fixed, thanks!
containerView.requestRectangleOnScreen(caretPix);It may be worth pointing out that this is the fallback. That is, the original request api that does not contain a focus source.
Was this path requested by the magnifier proposal?
I've added a comment clarifying that this is a fallback.
As for whether the fallback should be applied, I think "yes" for text-cursor following, and "maybe" for input-focus following (out of scope for this CL, but I've got a follow-up tech clean-up on that feature coming).
I didn't find an answer to this question explicitly in the proposal, and would like @ald...@google.com to weigh in, but the code examples for [TextView](https://docs.google.com/document/d/1WuE6b46k7mltgCVIeqHGR93wumEQFI1EhtRujacLPHE/edit?tab=t.0#heading=h.dy512rmzmes4) and [FullscreenMagnificationController](https://docs.google.com/document/d/1WuE6b46k7mltgCVIeqHGR93wumEQFI1EhtRujacLPHE/edit?tab=t.0#heading=h.se2dji6116wp) imply that the fallback path should be treated equivalently to a text cursor move.
When I follow up with Input Focus following, which of the following options should I take?
1. Status-quo: input-focus following is Chrome-only, new 36.1 request API always used if available, fallback path guarded behind default-disable flag.
2. Similar to this: input-focus following is in all embedders guarded by a Finch flag, 36.1 API is used if available, fallback path used if not.
3. Similar to proposal: same as option 2, but no requestRectangleOnScreen call made for input focus if the 36.1 source parameter API is not available.
4. Hybrid: Use the same Finch flag to guard both text-cursor and input-focus following, with a separate flag for using the fallback API for input-focus.
I think any of options 2 through 4 are an improvement on my initially-implemented option 1, so I should do one of them in the follow-up, and which one chosen has small impacts on this CL (basically, "what should I name the Finch flag"). Personally, I lean to option 3 (closest to the proposal, text-cursor uses the fallback API, input-focus does not), but given the timeline constraints it might be best to put both input-focus and text-cursor under the same Finch experiment rather than their current separate flags.
gfx.mojom.Rect? insertion_marker;Should this be called caret_bounds?
I opted to match the Android `CursorAnchorInfo` name here, since the Mojo struct explicitly documents itself as a representation of that object.
insertion_marker_info = std::move(focus_rect);Are you sure `focus_rect` is the right choice here?
What is `bounding_box_rect`? What happens if anchor is first? Should we use `anchor_rect` then? If you have a good understanding of
GetSelectionBoundsInWindow
then that works for me, but we want to get this part nailed down.
When I wrote this code, what I found out was that if you don't have a selection (e.g. blinking caret), `focus_rect` and `anchor_rect` are the same rectangle, the caret. If you are selecting text, `anchor_rect` is the fixed point where the text-selection process started, while `focus_rect` is the movable point where the text-selection grows or shrinks (and thus the one I'd expect users would want to be able to see as it moves). In both cases, `bounding_box_rect` is the rectangle surrounding the entire selection.
That said, running this build on my dev phone, Magnification doesn't actually follow the focus rect, and I'm not sure if that's because I somehow got the definitions swapped or because the event is "cursor anchor moved" and doesn't fire for selection changes. I'll look into that more tomorrow.
Related details
- Amanda Lin Dietz
- Bo Liu
- David Tseng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Tseng (Gerrit)
David Tseng added 3 comments![]( "Open in Gerrit")
containerView.requestRectangleOnScreen(caretPix);Aaron MossIt may be worth pointing out that this is the fallback. That is, the original request api that does not contain a focus source.
Was this path requested by the magnifier proposal?
I've added a comment clarifying that this is a fallback.
As for whether the fallback should be applied, I think "yes" for text-cursor following, and "maybe" for input-focus following (out of scope for this CL, but I've got a follow-up tech clean-up on that feature coming).
I didn't find an answer to this question explicitly in the proposal, and would like @ald...@google.com to weigh in, but the code examples for [TextView](https://docs.google.com/document/d/1WuE6b46k7mltgCVIeqHGR93wumEQFI1EhtRujacLPHE/edit?tab=t.0#heading=h.dy512rmzmes4) and [FullscreenMagnificationController](https://docs.google.com/document/d/1WuE6b46k7mltgCVIeqHGR93wumEQFI1EhtRujacLPHE/edit?tab=t.0#heading=h.se2dji6116wp) imply that the fallback path should be treated equivalently to a text cursor move.
When I follow up with Input Focus following, which of the following options should I take?
1. Status-quo: input-focus following is Chrome-only, new 36.1 request API always used if available, fallback path guarded behind default-disable flag.
2. Similar to this: input-focus following is in all embedders guarded by a Finch flag, 36.1 API is used if available, fallback path used if not.
3. Similar to proposal: same as option 2, but no requestRectangleOnScreen call made for input focus if the 36.1 source parameter API is not available.
4. Hybrid: Use the same Finch flag to guard both text-cursor and input-focus following, with a separate flag for using the fallback API for input-focus.I think any of options 2 through 4 are an improvement on my initially-implemented option 1, so I should do one of them in the follow-up, and which one chosen has small impacts on this CL (basically, "what should I name the Finch flag"). Personally, I lean to option 3 (closest to the proposal, text-cursor uses the fallback API, input-focus does not), but given the timeline constraints it might be best to put both input-focus and text-cursor under the same Finch experiment rather than their current separate flags.
Got it; yup, aldietz should probably weigh in here as I'm not sure what's expected on the magnifier side of things currently.
gfx.mojom.Rect? insertion_marker;Aaron MossShould this be called caret_bounds?
I opted to match the Android `CursorAnchorInfo` name here, since the Mojo struct explicitly documents itself as a representation of that object.
I only pointed it out because the naming is not clear but if that's borrowed from Android, then, I'l assume that's on me.
insertion_marker_info = std::move(focus_rect);Aaron MossAre you sure `focus_rect` is the right choice here?
What is `bounding_box_rect`? What happens if anchor is first? Should we use `anchor_rect` then? If you have a good understanding of
GetSelectionBoundsInWindow
then that works for me, but we want to get this part nailed down.
When I wrote this code, what I found out was that if you don't have a selection (e.g. blinking caret), `focus_rect` and `anchor_rect` are the same rectangle, the caret. If you are selecting text, `anchor_rect` is the fixed point where the text-selection process started, while `focus_rect` is the movable point where the text-selection grows or shrinks (and thus the one I'd expect users would want to be able to see as it moves). In both cases, `bounding_box_rect` is the rectangle surrounding the entire selection.
That said, running this build on my dev phone, Magnification doesn't actually follow the focus rect, and I'm not sure if that's because I somehow got the definitions swapped or because the event is "cursor anchor moved" and doesn't fire for selection changes. I'll look into that more tomorrow.
Yeah, I think the boolean is interesting. In DOM terms, anchor and focus do behave as you state above, but when the selection is reversed or programmatically set, we may hit strange cases. e.g. after a select all, line, word, do we want to move to the focus or anchor? It's not clear in those cases that focus is expected.
Related details
- Aaron Moss
- Amanda Lin Dietz
- Bo Liu
Aaron Moss (Gerrit)
Aaron Moss added 3 comments![]( "Open in Gerrit")
gfx.mojom.Rect? insertion_marker;Aaron MossShould this be called caret_bounds?
David TsengI opted to match the Android `CursorAnchorInfo` name here, since the Mojo struct explicitly documents itself as a representation of that object.
I only pointed it out because the naming is not clear but if that's borrowed from Android, then, I'l assume that's on me.
One relevant note is that Android keeps individual fields rather than a `Rect`, but the Builder class does name its setter [setInsertionMarkerLocation](https://developer.android.com/reference/android/view/inputmethod/CursorAnchorInfo.Builder#setInsertionMarkerLocation(float,%20float,%20float,%20float,%20int)) -- would you prefer `insertion_marker_location` for this field? I thought the extra verbosity over `insertion_marker` didn't pay off in clarity, but I don't feel strongly about it.
CalculateSelectionBounds(anchor_root_frame, focus_root_frame,I found these flipped params in existing code while looking into David's questions about focus vs. anchor on selection. They've been the way they are for years, so I'm running a fresh dry-run for the change, but the [declaration of `CalculateSelectionBounds`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/web_frame_widget_impl.cc;drc=cdf3ed32a94019461b37aa1f4a517847eb0989da;l=4662) has the parameters as anchor-then-focus, in line with my change, and this CL now follows focus instead of anchor (as intended).
insertion_marker_info = std::move(focus_rect);Aaron MossAre you sure `focus_rect` is the right choice here?
What is `bounding_box_rect`? What happens if anchor is first? Should we use `anchor_rect` then? If you have a good understanding of
GetSelectionBoundsInWindow
then that works for me, but we want to get this part nailed down.
David TsengWhen I wrote this code, what I found out was that if you don't have a selection (e.g. blinking caret), `focus_rect` and `anchor_rect` are the same rectangle, the caret. If you are selecting text, `anchor_rect` is the fixed point where the text-selection process started, while `focus_rect` is the movable point where the text-selection grows or shrinks (and thus the one I'd expect users would want to be able to see as it moves). In both cases, `bounding_box_rect` is the rectangle surrounding the entire selection.
That said, running this build on my dev phone, Magnification doesn't actually follow the focus rect, and I'm not sure if that's because I somehow got the definitions swapped or because the event is "cursor anchor moved" and doesn't fire for selection changes. I'll look into that more tomorrow.
Yeah, I think the boolean is interesting. In DOM terms, anchor and focus do behave as you state above, but when the selection is reversed or programmatically set, we may hit strange cases. e.g. after a select all, line, word, do we want to move to the focus or anchor? It's not clear in those cases that focus is expected.
"Looking into it more tomorrow" took most of the day, but it turns out that `GetSelectionBoundsInWindow()` had a years-old bug swapping anchor and focus in one of the calls it makes (now fixed and commented in this CL).
I played around a bit with the build, and it seems like the only time the caret moves from the anchor and not the focus is when you've been selecting one direction and then navigate the other direction (e.g. select-backward then move caret forward), which is a jerky motion anyway. I'd appreciate your perspective as an assistive technology user, but I still think "focus is the one that's being actively moved, focus is the one that should be magnified".
Full description of focus behaviour on keyboard navigation:
- select all: anchor is beginning of text, focus is end
- select to end-of-line: anchor is previous caret position, focus is end-of-line
- select to start-of-line: anchor is previous caret position, focus is start-of-line
- select line-up or down: anchor is previous caret position, focus is equivalent position one line up or down
- select next word: anchor is previous caret position, focus is end of next word
- select previous word: anchor is previous caret position, focus is beginning of previous word.
- When abandoning the selection and returning to caret navigation by hitting the arrow keys without shift: up and down arrow move from the focus position, left and right move from the first and last position in the selection, respectively (I assume RTL languages reverse this, but didn't test it).
Related details
- Amanda Lin Dietz
- Bo Liu
- David Tseng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |