Skip to content
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

Implementation of EditableTextState._scheduleShowCaretOnScreen #104500

Open
Oleksa5 opened this issue May 24, 2022 · 8 comments · Fixed by #102992
Open

Implementation of EditableTextState._scheduleShowCaretOnScreen #104500

Oleksa5 opened this issue May 24, 2022 · 8 comments · Fixed by #102992
Labels
a: text input Entering text in a text field or keyboard related problems c: proposal A detailed proposal for a change to Flutter f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team-design Owned by Design Languages team triaged-design Triaged by Design Languages team

Comments

@Oleksa5
Copy link

Oleksa5 commented May 24, 2022

Currently it's not possible to select more text than it's visible in a scrollable. Because the function is always trying to show a padded caret rect, which for selected text happens to be at the beginning of the selection. This is because _FloatingCursorPainter.paint skips painting for non-collapsed selection and updating the cashed caret rect is performed at the end of the painting via callback to RenderEditable.

Visually there is no caret on non-collapsed selection, then what to show? Why should the beginning of the selection be treated as the caret position and not the end? Maybe the function should have another name and EditableTextState._currentCaretRect be updated to null . Or _currentCaretRect should be updated even for non-collapsed selection and placed at the extent of the selection.

Probably the simplest solution would be to add

final Rect rectToReveal;
final TextSelection selection = textEditingValue.selection;
if (selection.isCollapsed) {
  rectToReveal = targetOffset.rect;
} else {
  final List<Rect> selectionBoxes = renderEditable.getBoxesForSelection(selection);
  rectToReveal = selection.baseOffset < selection.extentOffset ?
    selectionBoxes.last : selectionBoxes.first;
}

and change

rect: caretPadding.inflateRect(targetOffset.rect)

to

rect: caretPadding.inflateRect(rectToReveal)

Also note there is another issue with scrolling while selecting text by dragging the mouse, but that has to do with the implementation of TextSelectionGestureDetectorBuilder.onDragSelectionUpdate.

@Oleksa5 Oleksa5 changed the title Implementation of EditableTextState._scheduleShowCaretOnScreen Implementation of EditableTextState._scheduleShowCaretOnScreen May 24, 2022
@maheshmnj maheshmnj added the in triage Presently being triaged by the triage team label May 25, 2022
@maheshmnj
Copy link
Member

Hi @Oleksa5, Thanks for filing the issue.

Currently it's not possible to select more text than it's visible in a scrollable

Could you please share a minimal code sample which shows that this behavior is currently not possible, along with the output of flutter doctor -v?

@maheshmnj maheshmnj reopened this May 25, 2022
@maheshmnj maheshmnj added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label May 25, 2022
@Oleksa5
Copy link
Author

Oleksa5 commented May 25, 2022

import 'package:flutter/material.dart';

void main() {
  const kPadding = 16.0;

  final entry = ColoredBox(
    color: Colors.white,
    child: Padding(
      padding: const EdgeInsets.all(kPadding),
      child: TextField(
        controller: TextEditingController(
          text: "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Duis fermentum in dolor ut ullamcorper. Quisque ac nunc facilisis, accumsan orci vel, dictum dui. Cras nec eleifend mauris. Aliquam erat volutpat. Proin orci neque, convallis at justo eget, lacinia porta tellus. Suspendisse euismod ipsum risus. Quisque cursus vitae diam at venenatis. Quisque interdum maximus dui. Aenean ac turpis risus. Donec sed augue imperdiet, finibus dui ut, ullamcorper orci. Nulla placerat augue id lectus pulvinar, eu sollicitudin ex viverra. Cras pulvinar gravida est at laoreet. Phasellus accumsan tortor vel eros fermentum, eget fringilla leo commodo. Sed ut ligula fermentum, vestibulum ipsum id, suscipit justo. Cras rhoncus gravida pulvinar. Phasellus lectus lorem, ultrices in ornare at, commodo non eros. Phasellus imperdiet malesuada aliquet. Etiam mi turpis, tincidunt a commodo a, accumsan ut nibh. Praesent eleifend sagittis ornare. Pellentesque a dui nec velit mollis pulvinar. Quisque ut tincidunt turpis, at ultricies justo. Ut commodo nibh a lectus laoreet, id congue turpis placerat. Sed nec tellus suscipit, congue tortor non, sodales urna. Praesent vehicula purus imperdiet bibendum condimentum. Proin mauris diam, feugiat laoreet sagittis ac, mattis quis justo. Morbi eu nisl sem. Phasellus sit amet pharetra dui, in accumsan risus. Cras egestas elementum quam feugiat feugiat. Aenean aliquet porta orci tempus ullamcorper. Pellentesque pulvinar sit amet nunc vel ornare. Donec sollicitudin, ex ut efficitur accumsan, odio nisi pellentesque orci, eu pellentesque diam nunc sed ex. Fusce commodo leo eu imperdiet ornare. Nam id volutpat nulla, nec tincidunt risus. Nunc interdum porta nibh, ut maximus velit pulvinar vel. Nunc vel eros est. Nunc non vestibulum lacus. Donec malesuada ultrices ultricies. Cras tempus dui mauris. Sed vehicula tincidunt molestie. Suspendisse pretium semper gravida. Etiam mollis mi gravida hendrerit feugiat. Curabitur eget auctor velit. Nullam ultrices, nunc ullamcorper fringilla interdum, elit erat venenatis lectus, molestie rutrum enim purus ac tellus. Cras lacinia est id mi facilisis, aliquam rutrum neque dignissim. Aliquam vitae eros mauris. Nam et ornare risus. Nam mollis lacinia sapien vitae rhoncus. Curabitur bibendum felis felis, id hendrerit metus cursus quis. Sed dignissim sem vitae scelerisque elementum. Etiam nec felis in risus bibendum aliquet nec in urna. Maecenas lacinia est quis malesuada ullamcorper. Sed blandit venenatis justo in ultrices. Donec euismod massa eget aliquam molestie. Donec at ipsum at justo feugiat tempor in ac nisl. Morbi a turpis est. Cras pretium varius nisl malesuada dapibus. Nulla facilisi. Proin lectus lorem, convallis vel blandit quis, ullamcorper id ex. Integer semper ante nec nisi dictum molestie ut nec velit. Phasellus vulputate eu ipsum nec suscipit. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Maecenas aliquet efficitur felis, et accumsan tortor suscipit sit amet. Fusce accumsan rhoncus sapien, at pellentesque ligula."
        ),
        style: const TextStyle(
          color: Colors.black87
        ),
        maxLines: null
      ),
    ),
  );

  runApp(
    MaterialApp(
      darkTheme: ThemeData.dark(),
      themeMode: ThemeMode.dark,
      home: Material(
        child: ListView(
          padding: const EdgeInsets.all(kPadding),
          children: [ 
            entry, 
            const SizedBox(height: kPadding),
            entry
          ]
        ),
      ),
    )
  );
}

Note that the second entry is not necessary to reveal the issue.

Doctor summary:

[√] Flutter (Channel stable, 3.0.1, on Microsoft Windows [Version 6.1.7601], locale uk-UA)
    • Flutter version 3.0.1 at C:\Users\Oleksa\dev\sdk\flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision fb57da5f94 (5 days ago), 2022-05-19 15:50:29 -0700
    • Engine revision caaafc5604
    • Dart version 2.17.1
    • DevTools version 2.12.2

[√] Android toolchain - develop for Android devices (Android SDK version 32.1.0-rc1)
    • Android SDK at C:\Users\Oleksa\AppData\Local\Android\sdk
    • Platform android-32, build-tools 32.1.0-rc1
    • Java binary at: C:\Program Files\Android\Android Studio\jre\bin\java
    • Java version OpenJDK Runtime Environment (build 11.0.11+9-b60-7590822)
    • All Android licenses accepted.

[√] Chrome - develop for the web
    • Chrome at C:\Program Files\Google\Chrome\Application\chrome.exe

[√] Visual Studio - develop for Windows (Visual Studio Community 2019 16.11.12)
    • Visual Studio at C:\Program Files (x86)\Microsoft Visual Studio\2019\Community
    • Visual Studio Community 2019 version 16.11.32407.337
    • Windows 10 SDK version 10.0.19041.0

[√] Android Studio (version 2021.1)
    • Android Studio at C:\Program Files\Android\Android Studio
    • Flutter plugin can be installed from:
       https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
       https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.11+9-b60-7590822)

[√] VS Code (version 1.67.2)
    • VS Code at C:\Users\Oleksa\AppData\Local\Programs\Microsoft VS Code
    • Flutter extension version 3.40.0

[√] Connected device (3 available)
    • Windows (desktop) • windows • windows-x64    • Microsoft Windows [Version 6.1.7601]
    • Chrome (web)      • chrome  • web-javascript • Google Chrome 101.0.4951.67
    • Edge (web)        • edge    • web-javascript • Microsoft Edge 88.0.705.81

[√] HTTP Host Availability
    • All required HTTP hosts are available

• No issues found!

@github-actions github-actions bot removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label May 25, 2022
@Oleksa5
Copy link
Author

Oleksa5 commented May 25, 2022

The suggested solution only works for selecting by dragging mouse, but not by moving handles. So there should probably be more profound changes to make scrolling selection works with handles as well.

TextSelectionOverlay._handleSelectionHandleChanged calls selectionDelegate.bringIntoView(textPosition), which would be enough, but there are two more similar calls that messed things up for handles. These are EditableTextState._scheduleShowCaretOnScreen and selectionDelegate.bringIntoView(textPosition) in _TextFieldState._handleSelectionChanged. And that is only for a single selection change.

One solution would be to add handle value to SelectionChangedCause enum and make TextSelectionOverlay._handleSelectionHandleChanged specify it when calling TextSelectionDelegate.userUpdateTextEditingValue, which can then avoid bringing selection end into view assuming that a caller knows better what point should be revealed. Another option would be to extend TextSelectionDelegate.userUpdateTextEditingValue's interface by adding one extra parameter explicitly telling whether it should reveal an update.

As for _TextFieldState._handleSelectionChanged, it seems that a part where bringIntoView called can be simply removed.

@maheshmnj
Copy link
Member

Thanks for the code sample @Oleksa5, this issue looks the same as #102484, Where you have two scrollable (ListView, TextField with dynamic height) However, I am labeling this issue as a proposal to implement the above changes. I will leave this for the team to decide.

cc: @justinmc

@maheshmnj maheshmnj added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. c: proposal A detailed proposal for a change to Flutter and removed in triage Presently being triaged by the triage team labels May 26, 2022
@justinmc
Copy link
Contributor

I agree that the root problem is probably the same. I'll take another look at a solution in #102992.

@justinmc
Copy link
Contributor

After investigating, I think my solution in #102992 of using Scrollable.of to compensate for the outer scrollable isn't enough. It will still be blocked by the problem detailed in this issue. If I try it without addressing this issue, I get this kind of bounce:

Screen.Recording.2022-05-27.at.4.04.46.PM.mov

Interestingly, I didn't see that bounce until I merged in master. So commit bd37072 didn't bounce and worked fine without considering this issue. Something must have changed in the last ~3 weeks.

Either way now, I think my PR will need to address both issues.

@justinmc
Copy link
Contributor

justinmc commented Oct 5, 2022

#102992 will merge shortly, and with my testing it seems to fix this issue on desktop but not on mobile.

@justinmc justinmc removed their assignment Oct 5, 2022
@justinmc
Copy link
Contributor

justinmc commented Oct 5, 2022

Didn't mean to close this, we should figure out what the problem is on mobile.

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 c: proposal A detailed proposal for a change to Flutter f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team-design Owned by Design Languages team triaged-design Triaged by Design Languages team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants