-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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.
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
[web] Move text editing nodes outside of shadowDOM #39688
Changes from all commits
b7f7f22
1aa89a2
f055c0b
d9f1dd1
dfc1bab
23b5b6c
546b92c
9586c58
f74442e
ae00e40
3b6f9eb
4f75b6b
b600dc6
df91ed5
d8f96cd
cc4ed23
de2e916
97555e5
3ed55b8
36a0822
bf7f94e
401bb1c
3815cea
251fa13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,20 +19,22 @@ import '../semantics.dart' show EngineSemanticsOwner; | |
/// It also takes into account semantics being enabled to fix the case where | ||
/// offsetX, offsetY == 0 (TalkBack events). | ||
ui.Offset computeEventOffsetToTarget(DomMouseEvent event, DomElement actualTarget) { | ||
// On top of a platform view | ||
if (event.target != actualTarget) { | ||
return _computeOffsetOnPlatformView(event, actualTarget); | ||
} | ||
// On a TalkBack event | ||
if (EngineSemanticsOwner.instance.semanticsEnabled && event.offsetX == 0 && event.offsetY == 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ditman is the conditional order ok to change here? Running into a TalkBack issue where the offsets aren't being calculated correctly for pointer events. The problem is arising because any event that crosses into the shadowDOM has a cc @yjbanov this change fixes the TalkBack issue you discovered in the Gallery app. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this change makes sense. I'm not sure I have any special criteria when I added the ordering in the first place, but in this case it seems we're going from "most special corner case" to "least special case", so I think this change makes a lot of sense (also, glad that the change is simple and not something involving changing the measurements/algos :P) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spoke with @htoor3 over a VC. Some notes:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update on this:
|
||
return _computeOffsetForTalkbackEvent(event, actualTarget); | ||
} | ||
|
||
final bool isTargetOutsideOfShadowDOM = event.target != actualTarget; | ||
if (isTargetOutsideOfShadowDOM) { | ||
return _computeOffsetRelativeToActualTarget(event, actualTarget); | ||
} | ||
// Return the offsetX/Y in the normal case. | ||
// (This works with 3D translations of the parent element.) | ||
return ui.Offset(event.offsetX, event.offsetY); | ||
} | ||
|
||
/// Computes the event offset when hovering over a platformView. | ||
/// Computes the event offset when hovering over any nodes that don't exist in | ||
/// the shadowDOM such as platform views or text editing nodes. | ||
/// | ||
/// This still uses offsetX/Y, but adds the offset from the top/left corner of the | ||
/// platform view to the glass pane (`actualTarget`). | ||
|
@@ -57,7 +59,7 @@ ui.Offset computeEventOffsetToTarget(DomMouseEvent event, DomElement actualTarge | |
/// | ||
/// Event offset relative to FlutterView = (offsetX + xP, offsetY + yP) | ||
// TODO(dit): Make this understand 3D transforms, https://github.com/flutter/flutter/issues/117091 | ||
ui.Offset _computeOffsetOnPlatformView(DomMouseEvent event, DomElement actualTarget) { | ||
ui.Offset _computeOffsetRelativeToActualTarget(DomMouseEvent event, DomElement actualTarget) { | ||
final DomElement target = event.target! as DomElement; | ||
final DomRect targetRect = target.getBoundingClientRect(); | ||
final DomRect actualTargetRect = actualTarget.getBoundingClientRect(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yjbanov would love to get your thoughts on if we can safely move the semantics tree outside of the shadowDOM and into its own node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current problematic implementation of semantics, this shouldn't break anything I know of. However, one issue with the current semantics tree is that it does not contain platform views. This creates some issues with traversal order. I was thinking in the future to slice the semantics tree and interleave it with the render tree such that platform views can be embedded into the semantics tree while preserving the paint order. To do that, the semantics tree would have to be inside the shadow root, otherwise the
<slot>
elements won't work.Having said that, I'm willing to cross that bridge when we get there. What's the motivation for moving it out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Primary motivation to move it out is to fix the password autofill issue for when semantics is enabled, since
<input>
s will be rendered in the semantics tree in that case and they'll face the same shadowDOM issues. AFAIK, the 2 nodes that host text editing elements would be whatever host we specify intext_editing.dart
and the semantics tree.