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

Implement Text Selection handles in TextBox for touch input #13107

Merged
merged 14 commits into from
Oct 31, 2023

Conversation

emmauss
Copy link
Contributor

@emmauss emmauss commented Oct 2, 2023

What does the pull request do?

Implements text selection handles in textbox that allow changing text selection by dragging. This is activated by non-mouse input, i.e. touch and pen. This provides similar text selection behavior to what users are familiar with on android and iOS.

Recording.2023-10-02.122506.mp4

In addition, the textbox context menu has been simplified and optimized for mobile.

Recording.2023-10-09.091959.mp4

Though what I've planned for this pr is all in, it's still subject to change, so comments on implementation details and recommendations are welcome.

What is the current behavior?

Currently, text can only be be selected by dragging the caret along the text. This makes it very inaccurate on touch devices, and text selection is easily reset by accidental touches.

What is the updated/expected behavior with this PR?

How was the solution implemented (if it's not obvious)?

SelectionHandles must be drawn over all other controls, and though visibility of the handles is affected by the current text cursor, it shouldn't be clipped. As such, a new visual layer with a higher z-index is used to host the handles. This has the side effect of selection handles drawing over hosted popups, which are drawn in the OverlayLayer. It's possible to add text controls to the OverlayLayer, as such, that layer is not suitable to host selection handles.

Checklist

Breaking changes

This is a behavioral breaking change.

Obsoletions / Deprecations

Fixed issues

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0040386-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@robloo
Copy link
Contributor

robloo commented Oct 2, 2023

Shouldn't this be done with adorners? They are specifically designed for cases like this.

I'm a little uncomfortable with how this was implemented with new canvas controls overlaying everything else. It doesn't seem to match XAML ideas.

The graphical design looks good though and this feature is a big plus for mobile!

@emmauss
Copy link
Contributor Author

emmauss commented Oct 2, 2023

Shouldn't this be done with adorners? They are specifically designed for cases like this.

I'm a little uncomfortable with how this was implemented with new canvas controls overlaying everything else. It doesn't seem to match XAML ideas.

The graphical design looks good though and this feature is a big plus for mobile!

Adorners currently can't be used in the any layer above the AdornerLayer, which includes OverlayLayer. It also has a composition bug, #10409 .

@robloo
Copy link
Contributor

robloo commented Oct 2, 2023

Adorners currently can't be used in the any layer above the AdornerLayer, which includes OverlayLayer

Why would the selection handles need to appear over the overlay layer? In Android they do the following:

  1. Selection handles are BELOW the overlay layer which contains cut/copy/paste (this can be confirmed by moving the overlay manually). So they accept adorner below overlay layer in concept. This isn't a problem because:
  2. When changing the position of the selection handles, the overlay cut/copy/paste controls hide themselves. They will re-appear after a few millisecond delay again when the selection handles are no longer being moved.
  3. The overlay cut/copy/paste controls are shown in a Y-position below the selection handles. The position of the overlay needs to account for the selection handles (this alone solves most of the issues)

Someone can check iOS as well but simply copying Android would allow Adorner usage.

It also has a composition bug, #10409 .

There a few other issues for adorners as well: #10701 and #11232. However, those bugs should not drive a "hacky" solution for a core feature. The correct path is to fix adorners. We don't want a temporary solution that will stick around in the code base for years I think?

@robloo
Copy link
Contributor

robloo commented Oct 2, 2023

A few more comments:

  1. It's possible to add text controls to the OverlayLayer, as such, that layer is not suitable to host selection handles.

    I believe that's too severe of a limitation. Several input-type controls have a "popup" that will appear as an overlay on certain platforms. This would not allow modification of text in this scenario using the seleciton handles which seems to be a rather big architecture flaw. This indicates another approach is needed.

  2. The Cut/Copy/Paste menu is going to need to be customized for mobile platforms I think. Instead of a vertical layout of the commands iOS and Android both use a horizontal layout. This is important due to mobile form factor and screen sizes. I think this is fairly easy to do with the new OnPlatform markup extension.

  3. Looking at some iOS documentation it appears they also side-step this issue by placing the overlay cut/copy/paste buttons ABOVE the selection in most cases. Again using a Y-position offset to avoid any intersection.

@emmauss
Copy link
Contributor Author

emmauss commented Oct 2, 2023

Adorners currently can't be used in the any layer above the AdornerLayer, which includes OverlayLayer

Why would the selection handles need to appear over the overlay layer? In Android they do the following:

1. Selection handles are BELOW the overlay layer which contains cut/copy/paste (this can be confirmed by moving the overlay manually). So they accept adorner below overlay layer in concept. This isn't a problem because:

2. When changing the position of the selection handles, the overlay cut/copy/paste controls hide themselves. They will re-appear after a few millisecond delay again when the selection handles are no longer being moved.

3. The overlay cut/copy/paste controls are shown in a Y-position below the selection handles. The position of the overlay needs to account for the selection handles (this alone solves most of the issues)

Someone can check iOS as well but simply copying Android would allow Adorner usage.

It also has a composition bug, #10409 .

There a few other issues for adorners as well: #10701 and #11232. However, those bugs should not drive a "hacky" solution for a core feature. The correct path is to fix adorners. We don't want a temporary solution that will stick around in the code base for years I think?

I know of 1 popular avalonia control toolkit that uses the overlay layler for managed dialogs, ie content dialogs. These dialogs can host text boxes. And the overlay layer is above the adorner layer.

@emmauss
Copy link
Contributor Author

emmauss commented Oct 2, 2023

@robloo Android uses popups for their copy-paste menu. These are windows on their own, similar to desktop

@robloo
Copy link
Contributor

robloo commented Oct 3, 2023

Android uses popups for their copy-paste menu. These are windows on their own, similar to desktop

Ok, even if they are 'windows' rather than an overlay my point stands about z-index. The selection handles are underneath them as shown below. If it works for Android I have to think it would work for Avalonia.... however

image

I know of 1 popular avalonia control toolkit that uses the overlay layler for managed dialogs, ie content dialogs. These dialogs can host text boxes. And the overlay layer is above the adorner layer.

Yea, true, the adorner layer would still have to render above the overlay layer so the selection handles could appear in all cases like this. I was hoping this could be avoided but it can't - overlay hosted controls need selection handles too in some cases.

This also might be a little bit new territory since UWP didn't have adorners. Perhaps that's why they don't fully work in composition renderer now.

Perhaps the solution is to:

  1. Make adorners work again
  2. Render adorners above overlay layer. Actually, any adorner created in an overlay should just work so there is clearly a missing feature here.
  3. Make cut/copy/paste buttons horizontal on mobile to save Y-axis space
  4. Move cut/copy/paste buttons by:
    a. Hiding them when selection handles are being dragged
    b. Only show cut/copy/paste overlay above/below the selection box in the Y-axis

TL;DR: I still think adorners should be used. They were designed exactly for things like this.

@emmauss
Copy link
Contributor Author

emmauss commented Oct 3, 2023

Android uses popups for their copy-paste menu. These are windows on their own, similar to desktop

Ok, even if they are 'windows' rather than an overlay my point stands about z-index. The selection handles are underneath them as shown below. If it works for Android I have to think it would work for Avalonia.... however

image

I know of 1 popular avalonia control toolkit that uses the overlay layler for managed dialogs, ie content dialogs. These dialogs can host text boxes. And the overlay layer is above the adorner layer.

Yea, true, the adorner layer would still have to render above the overlay layer so the selection handles could appear in all cases like this. I was hoping this could be avoided but it can't - overlay hosted controls need selection handles too in some cases.

This also might be a little bit new territory since UWP didn't have adorners. Perhaps that's why they don't fully work in composition renderer now.

Perhaps the solution is to:

1. Make adorners work again

2. Render adorners above overlay layer. Actually, any adorner created in an overlay should just work so there is clearly a missing feature here.

3. Make cut/copy/paste buttons horizontal on mobile to save Y-axis space

4. Move cut/copy/paste buttons by:
   a. Hiding them when selection handles are being dragged
   b. Only show cut/copy/paste overlay above/below the selection box in the Y-axis

TL;DR: I still think adorners should be used. They were designed exactly for things like this.

3 and 4 are on my list of things to do for touch. 1 and 2 will need @kekekeks input. I think OverlayLayer was added after Adorners, so it was placed above so as not to break stuff.

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0040570-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@emmauss
Copy link
Contributor Author

emmauss commented Oct 9, 2023

@robloo Added mobile themed context flyouts for textbox

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0040578-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@@ -1470,7 +1459,7 @@ protected override void OnPointerPressed(PointerPressedEventArgs e)
var text = Text;
var clickInfo = e.GetCurrentPoint(this);

if (text != null && clickInfo.Properties.IsLeftButtonPressed &&
if (text != null && (e.Pointer.Type == PointerType.Mouse || e.ClickCount >= 2) && clickInfo.Properties.IsLeftButtonPressed &&
Copy link
Member

@MrJul MrJul Oct 9, 2023

Choose a reason for hiding this comment

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

I'm not sure what the new condition does, but it seems incompatible with the code inside the if branch: isTouch && e.ClickCount == 1 below can now never be true

Edit: this prevents setting the caret manually with touch without the handles once you have a selection.

Copy link
Member

Choose a reason for hiding this comment

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

Unresolving this as dragging the caret itself doesn't work (see my comment below).

@MrJul
Copy link
Member

MrJul commented Oct 9, 2023

There a few other issues for adorners as well: #10701 and #11232. However, those bugs should not drive a "hacky" solution for a core feature. The correct path is to fix adorners. We don't want a temporary solution that will stick around in the code base for years I think?

I completely agree with this statement. We're adding a public layer here to work around adorners issues, but they're really made for this case. Even when adorners get fixed, the current solution might have to stay, which really isn't ideal.


Apart from that, this PR is a great and much needed improvement for mobile!

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0040840-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@emmauss emmauss force-pushed the text_selector branch 2 times, most recently from 4f98c74 to 64f947c Compare October 17, 2023 15:10
@emmauss
Copy link
Contributor Author

emmauss commented Oct 19, 2023

2023-10-19-15-42-07.mp4

Here it is. Note that the caret still moves, but only when I'm releasing my finger. In all other apps, the caret moves with the finger (and displays a zoomed text, but that's a different improvement).

Ok, I didn't account for that. There may be issues with scrolling, but I'll try to alleviate it.

@emmauss
Copy link
Contributor Author

emmauss commented Oct 23, 2023

To clarify the "with selection" part: once you have a selection, you can unselect/move the caret but it takes two taps: one to dismiss the context menu (that opens automatically) and another one to move the caret elsewhere. One tap should be sufficient to both close the menu and move the caret (at least that's the behavior in all other apps).

That said, I understand if you want that change to be part of another PR. The current behavior is already way, way better than before.

This behavior would need #13351 first, so will update after that's merged.

@emmauss
Copy link
Contributor Author

emmauss commented Oct 27, 2023

@MrJul Should be good now. could you try again?

@MrJul
Copy link
Member

MrJul commented Oct 28, 2023

@MrJul Should be good now. could you try again?

Just tried for a few minutes, touch text selection and caret moving feels quite good, kudos!

You even fixed the outer scrollviewer preventing proper touch scrolling inside textboxes after a while (#13351 I presume), which was quite annoying.

@maxkatz6 maxkatz6 added this pull request to the merge queue Oct 31, 2023
@robloo
Copy link
Contributor

robloo commented Oct 31, 2023

Awesome work again!

Finally, what was decided about adorners? Will this be adjusted later when adorners work in overlays and the bugs are fixed?

Merged via the queue into master with commit eb5fdd2 Oct 31, 2023
@maxkatz6 maxkatz6 deleted the text_selector branch October 31, 2023 11:17
@hez2010
Copy link
Contributor

hez2010 commented Oct 31, 2023

Several issues after testing on a real Android device:

  • It's very hard to enter the selection mode, in most cases the selection will be dismissed immediately.
  • On a multiple line textbox with a scrollbar, you cannot scroll the text area to adjust selection carets and the selection will dismiss after you scroll the textarea.

@emmauss
Copy link
Contributor Author

emmauss commented Oct 31, 2023

Awesome work again!

Finally, what was decided about adorners? Will this be adjusted later when adorners work in overlays and the bugs are fixed?

Hopefully, yes.

@emmauss
Copy link
Contributor Author

emmauss commented Oct 31, 2023

Several issues after testing on a real Android device:

* It's very hard to enter the selection mode, in most cases the selection will be dismissed immediately.

* On a multiple line textbox with a scrollbar, you cannot scroll the text area to adjust selection carets and the selection will dismiss after you scroll the textarea.

Selection mode is entered by either tapping the text to set the handle, then tapping the handle, or holding on the text. Drag selection for touch has been removed due to how it interferes with gestures.
Scrolling on a multiline textbox is done simply by moving the touch vertically when you press. Moving it horizontally immediately you press will disable scrolling for that pointer and instead allow changing the caret position. For a single line textbox that scrolls horizontally, using the handles to move selection is sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants