Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Limit selection change to focused node on Windows - #38634

Merged
yaakovschectman merged 6 commits into
flutter-team-archive:mainfrom
yaakovschectman:uiatext_focus
Jan 6, 2023
Merged

Limit selection change to focused node on Windows#38634
yaakovschectman merged 6 commits into
flutter-team-archive:mainfrom
yaakovschectman:uiatext_focus

Conversation

@yaakovschectman

@yaakovschectman yaakovschectman commented Jan 4, 2023

Copy link
Copy Markdown
Contributor

When the text selection of a node is changed, we should only set the AX tree's selection to that of the focused node. Currently, we set it for every node in the update, which means that whichever node happens to be last will have the selection that actually persists, even if it is not the focused node. The order of nodes when iterating through an update is meaningless. The focused flag is added to one unit test as well.

Part of flutter/flutter#116219

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@yaakovschectman yaakovschectman self-assigned this Jan 4, 2023
@yaakovschectman
yaakovschectman marked this pull request as ready for review January 4, 2023 17:57
loic-sharma
loic-sharma reviewed Jan 4, 2023
View reviewed changes
Comment thread shell/platform/windows/accessibility_bridge_windows.cc
case ui::AXEventGenerator::Event::DOCUMENT_SELECTION_CHANGED: {
ui::AXNode::AXID focus_id = GetAXTreeData().sel_focus_object_id;
auto focus_delegate =
GetFlutterPlatformNodeDelegateFromID(focus_id).lock();

@loic-sharma loic-sharma Jan 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we guaranteed to get a focus delegate? I see the corresponding Chromium logic checks if it was able to find a focus object: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/accessibility/browser_accessibility_manager_win.cc;l=235-237?q=case%20ui::AXEventGenerator::Event::DOCUMENT_SELECTION_CHANGED&ss=chromium

@loic-sharma loic-sharma Jan 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a test for this scenario? If no, can we add one?

loic-sharma
loic-sharma reviewed Jan 4, 2023
View reviewed changes
Comment thread shell/platform/windows/accessibility_bridge_windows.cc
ax::mojom::Event::kChildrenChanged);
break;
case ui::AXEventGenerator::Event::DOCUMENT_SELECTION_CHANGED:
case ui::AXEventGenerator::Event::DOCUMENT_SELECTION_CHANGED: {

@loic-sharma loic-sharma Jan 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we add a comment similar to Chromium's comment? https://source.chromium.org/chromium/chromium/src/+/main:content/browser/accessibility/browser_accessibility_manager_win.cc;l=230-234?q=case%20ui::AXEventGenerator::Event::DOCUMENT_SELECTION_CHANGED&ss=chromium

@loic-sharma loic-sharma Jan 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this part of the comment make sense for us too?

This is because the focus is the only endpoint that can move, and because the caret (if present) is at the focus.

@yaakovschectman yaakovschectman Jan 5, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do not think so, as we do not have the carat as its own node.

@loic-sharma loic-sharma Jan 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps I'm misunderstanding, but my reading of that comment is that both the selection & the caret - if they exist - must be on the focus node.

@yaakovschectman
yaakovschectman merged commit fd5a96e into flutter-team-archive:main Jan 6, 2023
@yaakovschectman
yaakovschectman deleted the uiatext_focus branch January 6, 2023 21:59
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 9, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 9, 2023
@engine-flutter-autoroll
fd5a96e Limit selection change to focused node on Windows (flutter-te…
874e6b8 
…am-archive/engine#38634)
auto-submit Bot pushed a commit to flutter/flutter that referenced this pull request Jan 9, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 03609b4 to b5513d7 (2 revisions) (#118186)
c87cfd7 
* fd5a96e Limit selection change to focused node on Windows (flutter-team-archive/engine#38634)

* b5513d7 Roll Dart SDK from 0b064bc49557 to cb29cb6d1d0f (12 revisions) (flutter-team-archive/engine#38688)
esouthren pushed a commit to esouthren/flutter that referenced this pull request Jan 15, 2023
@engine-flutter-autoroll @esouthren
Roll Flutter Engine from 03609b4 to b5513d7 (2 revisions) (flutter#11…
e2a513b 
…8186)

* fd5a96e Limit selection change to focused node on Windows (flutter-team-archive/engine#38634)

* b5513d7 Roll Dart SDK from 0b064bc49557 to cb29cb6d1d0f (12 revisions) (flutter-team-archive/engine#38688)
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Jan 16, 2023
@esouthren @guidezpl
@engine-flutter-autoroll @Jasguerrero @darrenaustin @asashour @goderbauer @gnprice @CicadaCinema @thkim1011 @Renzo-Olivares Renzo Olivares @srawlins @peixinli @moffatman @mraleph @christopherfujino @fluttergithubbot @camsim99 @LongCatIsLooong @drewroengoogle @jason-simmons @nehalvpatel @gmackall @mackall-work @M97Chahboun @a-wallen a-wallen @morriskurz @LucasXu0 @jmagman @helin24 @TahaTesser @nshahan @yjbanov @bleroux @bdero @loic-sharma Jonah Williams @Time1ess @sigurdm @rydmike @dependabot @isaacscha @andrewkolos @QuncCccccc @joshualitt @awesometic @parlough @IvoB1987 @godofredoc @bernaferrari @IchordeDionysos @HansMuller @vashworth
M3 Button padding adjustments (#118449