Conversation
flutter-dashboard
bot
added
a: text input
|
Does this prevent edge scrolling if the text field yields focus once the touch point goes outside of its bounds? |
|
I've just merged in master so we get the edge scrolling PR. The answer is yes, but I think the behavior is desired for that case, even if it is a little weird. Without the MouseRegion that unfocuses on exit, the edge scrolling behavior is the same with/without this PR. Here is a video of me testing edge scrolling on this PR using the the example app from the issue #93143. Screen.Recording.2021-11-18.at.4.04.23.PM.mov |
|
Gold has detected about 1 new digest(s) on patchset 3. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| void onDragSelectionUpdate(DragStartDetails startDetails, DragUpdateDetails updateDetails) { | ||
| if (!delegate.selectionEnabled) | ||
| return; | ||
| if (!editableText.widget.focusNode.hasFocus) |
There was a problem hiding this comment.
I would prefer remove these lines:
flutter/packages/flutter/lib/src/widgets/editable_text.dart
Lines 2344 to 2346 in ca14d85
instead of explicitly preventing drag update events from getting through when the text field is unfocused. Even if the text field is unfocused you may still want edge scroll?
Unfortunately some tests that use EditableText are using tap to focus the input field and removing the said lines breaks those tests but that should be easy to fix? Not sure how many tests are doing that though.
There was a problem hiding this comment.
Oh another thing is current the text selection highlight is only visible if the text field has the focus. Maybe we should change that so when the selection is being changed the highlight is still visible.
There was a problem hiding this comment.
@vs-krasnov Do you have any opinion on this as the original author of the issue? What if the field unfocused but the selection continued to be visible and to change with the drag?
There was a problem hiding this comment.
@justinmc thanks for taking care of this issue so quickly ❤️
IMHO, once a text field becomes unfocused, we shouldn't change the state of this text field anymore based on future drag events.
I wouldn't expect the selection to continue for unfocused text fields.
There was a problem hiding this comment.
It looks like Chrome does continue to allow edge scrolling but doesn't show the highlight:
Does it work for everyone if we go with that behavior in Flutter? I've updated the PR to do that. Assuming tests go green.
HTML example code
<!doctype html>
<html class="no-js" lang="">
<head>
<meta charset="utf-8">
<title></title>
<meta name="description" content="">
<meta name="viewport" content="width=device-width, initial-scale=1">
<meta property="og:title" content="">
<meta property="og:type" content="">
<meta property="og:url" content="">
<meta property="og:image" content="">
<link rel="manifest" href="site.webmanifest">
<link rel="apple-touch-icon" href="icon.png">
<!-- Place favicon.ico in the root directory -->
<link rel="stylesheet" href="css/normalize.css">
<link rel="stylesheet" href="css/main.css">
<meta name="theme-color" content="#fafafa">
<style>
.box {
width: 100px;
height: 100px;
background-color: dodgerblue;
}
</style>
</head>
<body>
<!-- Add your site or application content here -->
<p>Hello world! This is HTML5 Boilerplate.</p>
<input type="number" />
<div class="box">Hover over this to unfocus</div>
<script src="js/vendor/modernizr-3.11.2.min.js"></script>
<script src="js/plugins.js"></script>
<script src="js/main.js"></script>
<!-- Google Analytics: change UA-XXXXX-Y to be your site's ID. -->
<script>
window.ga = function () { ga.q.push(arguments) }; ga.q = []; ga.l = +new Date;
ga('create', 'UA-XXXXX-Y', 'auto'); ga('set', 'anonymizeIp', true); ga('set', 'transport', 'beacon'); ga('send', 'pageview')
</script>
<script src="https://www.google-analytics.com/analytics.js" async></script>
<script>
document.querySelector('.box').addEventListener('mouseenter', () => {
document.activeElement.blur();
});
</script>
</body>
</html>There was a problem hiding this comment.
It makes sense to follow the existing platform's behavior 👍
|
Gold has detected about 7 new digest(s) on patchset 4. |
justinmc
force-pushed
the
drag-after-unfocused-field
branch
from
a99a244 to
f135d55
Compare
|
@LongCatIsLooong Ready for re-review. |
| // This will show the keyboard for all selection changes on the | ||
| // EditableWidget, not just changes triggered by user gestures. | ||
| requestKeyboard(); | ||
| if (!_hasFocus) { |
There was a problem hiding this comment.
nit: I think requestFocus does nothing if the node is already the primary focus so the if check is not needed?
There was a problem hiding this comment.
I'm not sure I understand the change. This part seems to say that when the text selection changes the field will keep requesting focus? Then if the user keeps dragging the selection the text field will regain focus?
…esture handler instead. Maybe should have more focuses in other gestures too?
|
Gold has detected about 7 new digest(s) on patchset 9. |
|
Talking to @LongCatIsLooong, it seems like this PR needs a bit of refactoring to be able to land with the correct behavior. The tap gesture detector in RenderEditable prevents us from being able to set focus in the gesture detectors. We need to move that out of RenderEditable before this solution can move forward. The goal is still the behavior agreed upon above. I'm going to put this PR back into draft mode. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
(triage): Are there still plans for this PR? |
|
Sorry I abandoned this PR for awhile. I'll get it up to date and then either finish it or close it for now depending on how relevant it still looks. |
|
I'm going to close this as I investigate refactoring TextSelectionControls as a part of my context menu work. Once that is done, I should reinvestigate if it makes this refactor easier. |
5 participants
Previously, it wasn't really possible to unfocus a field during a drag gesture; the field would be immediately re-focused at the next dragupdate and the selection changes would continue. This PR stops listening to drag events when the field is not focused.
Fixes #93143