Skip to content

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Mar 20, 2024

Description

This PR adds shortcuts related to numpad keys on Linux.

Related Issue

Linux side for #144936

Tests

Adds 2 tests.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Mar 20, 2024
@bleroux bleroux requested a review from justinmc March 20, 2024 16:00
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍. Numlock never crossed my mind for these kinds of shortcuts. Thanks for adding support.

But one question. Do you think it would be useful to add tests in editable_text_test or text_field_test to show that these keyboard shortcuts actually work in a text input? Or I guess you're using existing Intents that are already tested, so maybe that's redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this gets automatically reset after the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is. I first wanted to reset it but my thinking was that it might hide a bug in test bindings if one day the keyboard state reset no more works as expected.

@bleroux
Copy link
Contributor Author

bleroux commented Mar 20, 2024

But one question. Do you think it would be useful to add tests in editable_text_test or text_field_test to show that these keyboard shortcuts actually work in a text input? Or I guess you're using existing Intents that are already tested, so maybe that's redundant.

Yes, existing intents are already tested, so it will introduce a lot of redundancy (there are 28 shortcuts in this Linux only PR and there will be even more in the Windows one I plan). While refactoring InputDecorator tests, I found there is a lot of redundancy in tests and that usually made them harder to understand (more difficult to spot what is really tested) and, for the M2 to M3 migration, it leads to a lot of tests failing on things that are not direcly related to the tests (for instance lot of tests were checking again and again the container height, the border position, the border width).

@bleroux bleroux force-pushed the linux_numpad_shortcuts branch 3 times, most recently from 394dd8c to 26bbaee Compare March 21, 2024 14:48
@bleroux bleroux force-pushed the linux_numpad_shortcuts branch from 26bbaee to 9d675be Compare March 21, 2024 19:38
@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 22, 2024
@auto-submit auto-submit bot merged commit 859eb2e into flutter:master Mar 22, 2024
@bleroux bleroux deleted the linux_numpad_shortcuts branch March 22, 2024 06:22
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 22, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 22, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 22, 2024
camsim99 pushed a commit to flutter/packages that referenced this pull request Mar 25, 2024
flutter/flutter@18340ea...14774b9

2024-03-22 [email protected] Roll Flutter Engine from
eba6e31498b8 to 09dadce76828 (1 revision) (flutter/flutter#145603)
2024-03-22 [email protected] Roll Flutter Engine from
f9a34ae0b14f to eba6e31498b8 (1 revision) (flutter/flutter#145598)
2024-03-22 [email protected] Intensive `if` chain refactoring
(flutter/flutter#145194)
2024-03-22 [email protected] Adds numpad navigation shortcuts for
Linux (flutter/flutter#145464)
2024-03-22 [email protected] Roll Flutter Engine from
5a12de1beab7 to f9a34ae0b14f (1 revision) (flutter/flutter#145581)
2024-03-22 [email protected] Roll Flutter Engine from
e2f324beac3b to 5a12de1beab7 (1 revision) (flutter/flutter#145578)
2024-03-22 [email protected] Replace
`RenderBox.compute*` with `RenderBox.get*` and add
`@visibleForOverriding` (flutter/flutter#145503)
2024-03-22 [email protected] Add some cross
references in the docs, move an example to a dartpad example
(flutter/flutter#145571)
2024-03-22 [email protected] Fix `BorderSide.none` requiring
explicit transparent color for `UnderlineInputBorder`
(flutter/flutter#145329)
2024-03-22 [email protected] Roll Flutter Engine from
a46a7b273a5b to e2f324beac3b (1 revision) (flutter/flutter#145576)
2024-03-21 [email protected] Fix nullability of
getFullHeightForCaret (flutter/flutter#145554)
2024-03-21 [email protected] Add a
`--no-gradle-generation` mode to the `generate_gradle_lockfiles.dart`
script (flutter/flutter#145568)
2024-03-21 [email protected] Roll Flutter Engine from
1b842ae58b3d to a46a7b273a5b (2 revisions) (flutter/flutter#145569)
2024-03-21 [email protected] Fixed race condition in
PollingDeviceDiscovery. (flutter/flutter#145506)
2024-03-21 [email protected] Roll Flutter Engine from
a2ed373fa70f to 1b842ae58b3d (1 revision) (flutter/flutter#145565)
2024-03-21 [email protected] Clarify AutomaticKeepAliveClientMixin semantics
in build method (flutter/flutter#145297)
2024-03-21 [email protected] Eliminate more window singleton usages
(flutter/flutter#145560)
2024-03-21 [email protected] `flutter test --wasm` support
(flutter/flutter#145347)
2024-03-21 [email protected] Roll Flutter Engine from
eb262e9c34db to a2ed373fa70f (2 revisions) (flutter/flutter#145556)
2024-03-21 [email protected] Roll Flutter Engine from
bad4a30e1c75 to eb262e9c34db (1 revision) (flutter/flutter#145555)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC
[email protected],[email protected],[email protected] on
the revert to ensure that a human
is aware of the problem.

To file a bug in Packages:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
auto-submit bot pushed a commit that referenced this pull request May 25, 2024
## Description

This PRs fixes a Web issue on Linux related to numpad keys.
In #145464, I introduced numpad shortcuts for Linux. These shortcuts work well on a desktop Linux application but they broke the Linux+Web numpad logic.

When I added these shortcuts, I expected them to not be active on Web (because I knew that on Web, those shortcuts are handled by the browser). But there is a trick: text editing shortcuts are still defined on Web but they are disabled at the editable text level so one can use them in components that are not `EditableText` (see #103377).
In this PR, I used the same approach than for other text editing shortcuts: when on web associate those shortcuts to the `DoNothingAndStopPropagationTextIntent` intent.

## Related Issue

Fixes #148447.

## Tests

Updates 2 tests.
Adds 2 tests.
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request May 31, 2024
## Description

This PRs fixes a Web issue on Linux related to numpad keys.
In flutter#145464, I introduced numpad shortcuts for Linux. These shortcuts work well on a desktop Linux application but they broke the Linux+Web numpad logic.

When I added these shortcuts, I expected them to not be active on Web (because I knew that on Web, those shortcuts are handled by the browser). But there is a trick: text editing shortcuts are still defined on Web but they are disabled at the editable text level so one can use them in components that are not `EditableText` (see flutter#103377).
In this PR, I used the same approach than for other text editing shortcuts: when on web associate those shortcuts to the `DoNothingAndStopPropagationTextIntent` intent.

## Related Issue

Fixes flutter#148447.

## Tests

Updates 2 tests.
Adds 2 tests.
m3c-fnicola pushed a commit to m3c-fnicola/flutter that referenced this pull request Jun 24, 2024
## Description

This PRs fixes a Web issue on Linux related to numpad keys.
In flutter#145464, I introduced numpad shortcuts for Linux. These shortcuts work well on a desktop Linux application but they broke the Linux+Web numpad logic.

When I added these shortcuts, I expected them to not be active on Web (because I knew that on Web, those shortcuts are handled by the browser). But there is a trick: text editing shortcuts are still defined on Web but they are disabled at the editable text level so one can use them in components that are not `EditableText` (see flutter#103377).
In this PR, I used the same approach than for other text editing shortcuts: when on web associate those shortcuts to the `DoNothingAndStopPropagationTextIntent` intent.

## Related Issue

Fixes flutter#148447.

## Tests

Updates 2 tests.
Adds 2 tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants