Fixes on widget.Entry

[fpabl0]

Description:

Fixes:

1. Allow copy content from a disabled widget.Entry using the keyboard.
2. Hide text selection on a disabled widget.Entry when it loses focus.
3. Do not show validation state if widget.Entry is disabled.
4. If CursorColumn is equal to selectColumn and CursorRow is equal selectRow in DragEnd event, reset selection (with [2] fixes Cursor in Disabled Entry #1880).
5. Reset selectKeyDown variable when widget.Entry loses its focus (fixes Text selection - Shift+Tab bug #1787).

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[andydotxyz]

Lots of great fixes, thanks.

I'm not sure about this one:

Allow copy content from a disabled widget.Entry using the keyboard.

As that requires we move disabled widgets back into the focus cycle... This seems like an anti-pattern as you should not be able to focus things that cannot be interacted - should you?

[fpabl0]

This seems like an anti-pattern as you should not be able to focus things that cannot be interacted - should you?

I agree with you, however widget.Entry is a special because it has the disabled state mixed with readonly state. Disabled entry can be interacted (text selection), and if we don't put disabled Entry into the focus cycle, text selection can't be erased on it (one of the current problems).

[andydotxyz]

text selection can't be erased on it (one of the current problems).

This seems like a tap handler issue not a keyboard one. Tapping should clear selection.

[fpabl0]

This seems like a tap handler issue not a keyboard one. Tapping should clear selection.

Tapping indeed clears selection, but it only happens if you tap on the disabled entry, if you tap outside of it, you can't clear the selection (current issue). As far as I can see, the only way to clear it, is when Entry loses focus, that is how it works when entry is enabled.

[andydotxyz]

It's hard to be objective here as I don't really understand the difference between a ReadOnly / Disabled Entry and a Label widget.

IMHO it's not terrible that you have to tap the entry to deselect when disabled, as you have to make the selection through taps as well...

[fpabl0]

It's hard to be objective here as I don't really understand the difference between a ReadOnly / Disabled Entry and a Label widget.

Hmm, considering that label does not have the functionality to select text, it is different from Entry. ReadOnly and Disabled entry are indeed very similar, but the first one should be focused and the last one no. I don't think it is wrong to have only disabled entry, but it should get the same benefits of a readonly entry.

IMHO it's not terrible that you have to tap the entry to deselect when disabled, as you have to make the selection through taps as well...

I don't follow that, sorry. That is not the way readonly (disabled here) inputs works, and it leads to a bit confusing interface, look this case:

[andydotxyz]

I don't think that the difference between read-only and disabled are meaningful.
An entry can either be edited (hence get focus and manage keystrokes) or it cannot (and so should not be in the tab-focus-list).

IMHO it's not terrible that you have to tap the entry to deselect when disabled, as you have to make the selection through taps as well...

I don't follow that, sorry.

Apart from using the mouse/touch gestures, how do you select text for a disabled entry?
My understanding is that you can't - so removing selection through tap is consistent.

[fpabl0]

An entry can either be edited (hence get focus and manage keystrokes) or it cannot (and so should not be in the tab-focus-list).

I agree with you. This PR does not change the tab-focus-list. It only uses the focus to be able to copy the content and unselect the text when you tap outside, all other characteristics remain the same.

Apart from using the mouse/touch gestures, how do you select text for a disabled entry?
My understanding is that you can't - so removing selection through tap is consistent.

Hmm, maybe I don't write it well, I mean this is not good:

disabled-text-selection.mp4

A good example of how it should work, is this github conversation, every comment block is readonly and you can select the text, copy it with the keyboard and unselect it when tapping outside. I think that is the behavior final users would expect, don't they?

[andydotxyz]

I agree with you. This PR does not change the tab-focus-list. It only uses the focus to be able to copy the content and unselect the text when you tap outside, all other characteristics remain the same.

I am confused then - how does the top line "Allow copy content from a disabled widget.Entry using the keyboard" work if you are not allowing the entry to be part of the focus list? keystrokes are only sent to the currently focused element.

A good example of how it should work, is this github conversation, every comment block is readonly

I disagree - the comment blocks are simple HTML text. On a web page all text is selectable - unlike in an application. I would consider these labels, not read-only entries. Web pages are documents transitioned into an application, whereas fyne apps are apps not documents. I don't think that it's a direct comparison.

[fpabl0]

I am confused then - how does the top line "Allow copy content from a disabled widget.Entry using the keyboard" work if you are not allowing the entry to be part of the focus list? keystrokes are only sent to the currently focused element.

Because the function that allow us to go to the next focusable widget, check again if it is disabled or not:

fyne/internal/app/focus_manager.go

Lines 117 to 125 in ec5bf01

	func (f *FocusManager) nextWithWalker(current fyne.Focusable, walker walkerFunc) fyne.Focusable {
	var next fyne.Focusable
	found := current == nil // if we have no starting point then pretend we matched already
	walker(f.content, func(obj fyne.CanvasObject, _ fyne.Position, _ fyne.Position, _ fyne.Size) bool {
	if w, ok := obj.(fyne.Disableable); ok && w.Disabled() {
	// disabled widget cannot receive focus
	return false
	}

I disagree - the comment blocks are simple HTML text. On a web page all text is selectable - unlike in an application.

Oh yes, you are right, sorry for the confusion (I hadn't realized the difference before). After testing some native apps, disabled inputs can't even be selected (I mean the text). However, I wonder if we should go so rigid in that direction.

Considering that most of the modern apps are web based (VSCode, Postman, Slack, Discord, MicrosoftTeams, MongoDB Compass...) and final users are familiarized with that kind of applications, don't you think that should be good to adopt part of this behavior to avoid final users get confused?

MongoDB Compass example:

mongo_select.mp4

[andydotxyz]

So to clarify what I think I am beginning to understand:
You're updating this so that disabled items can be focused if tapped, but they cannot be by tabbing in to?

And, if that's correct, then by extension selecting inside a disabled entry will de-focus whatever was focused before?

[fpabl0]

You're updating this so that disabled items can be focused if tapped, but they cannot be by tabbing in to?

Yes, but not all the items, just the Entry or more general the ones which has SelectedText() method.

And, if that's correct, then by extension selecting inside a disabled entry will de-focus whatever was focused before?

Yes, that's correct, just like if you tapped outside.

[andydotxyz]

Thanks for the clarification. I'd like to get @toaster opinion on this as he knows focus really well.
What you describe sounds like a reasonable trade-off, but somehow I feel uneasy. Maybe it's checking for SelectedText(), or maybe it's because this will enable developers to focus items that were previously not possible.

[fpabl0]

Thanks for the clarification.

You're welcome.

I'd like to get @toaster opinion on this as he knows focus really well.

Great! :)

What you describe sounds like a reasonable trade-off, but somehow I feel uneasy.

Yes, that's right, I have the same feeling at code level, but at final user level I am satisfied 😅.

Maybe it's checking for SelectedText(), or maybe it's because this will enable developers to focus items that were previously not possible.

Initially I tried checking directly for *widget.Entry, but it results in a cyclic import. Then I thought that maybe checking for SelectedText() should be enough, because if a widget has this method it would probably need the same behavior.

[andydotxyz]

I have added this to today's agenda to chat it through

[andydotxyz]

Ok I think we’re good. Just needs develop pulled in for the new tests to pass