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

_demo: possible to delete characters even when Text Input widget is disabled #9

Closed
mewmew opened this issue Dec 30, 2020 · 7 comments
Closed
Labels
bug Something isn't working

Comments

@mewmew
Copy link

mewmew commented Dec 30, 2020

In the demo application it is currently possible to delete characters (by pressing the [backspace] or [delete] key) typed into the Text Input widget, even when the widget is disabled.

Steps to reproduce:

  1. Navigate to the Text Input widget in the demo application.
  2. Enter text (e.g. "abcd").
  3. Disable the widget.
  4. Now, use the mouse to position the text carret in the Text Input widget.
  5. Press the [backspace] or the [delete] key and notice that a character is deleted from the disabled widget.

screenshot_2020-12-31_00:25:04

@blizzy78 blizzy78 added the bug Something isn't working label Dec 30, 2020
@blizzy78
Copy link
Contributor

Whoops, fixed, thanks!

@mewmew
Copy link
Author

mewmew commented Dec 31, 2020

@blizzy78, wow. incredible response time!

If possible, I would like to open up for discussion on a different approach for the implementation of the Disable operation.

Currently, the widget is checking if it is disabled in multiple places (e.g. doBackspace, doDelete, charInputState), which works. However, it could result in the introduction of similar issues in the future, if the widget is extended with new functionality.

Another approach could be to filter out (not forward) events when the widget is disabled. As such, the code to implement the Disabled mechanics would be confined to a single (or very few) places of the code; e.g. the event dispatcher. By not forwarding mouse button clicks and key press events to disabled widgets, they should not be able to change based on such events.

At least this is the high-level idea, I'd like to discuss with you as you have keen insight into the UI implementation.

To give a concrete example, should the disabled Text Input widget still be selectable? I can envision two options for this, both which have merit. Either it is not selectable, and Disabled widgets are considered rendered but not interactable. The other option is that it is selectable, and interactable in a read-only state but not read-write. This would make it possible to e.g. select and copy text from disabled Text Input widgets; which is a great feature! However, thinking further on this regard, perhaps then it would be too limiting to filter events as an implementation of the Disabled operation, as this would not make it possible to select the Text Input widget (to place the text carret and select text) in a read-only matter.

Food for thought.

Cheers,
Robin

@blizzy78
Copy link
Contributor

I may be a bit biased because of my day job (lots of Eclipse RCP and SWT work), but I believe that UI widgets are generally more useful if you can still somehow interact with them even though you're not able to modify them. As in the text input example, I think you should still be able to position the cursor, select and copy text. For lists, you should still be able to scroll around and see all the entries, and so on.

In that regard, maybe the "disabled" flag should get a better name, such as "read-only" (not really a good fit for push buttons, though.) In any case, the flag is not supposed to cover event handling at all. As you mentioned, a read-only text input widget still needs to be able to receive events such as mouse clicks so that the text cursor can be positioned.

I agree that having to check the flag in multiple places may introduce similar bugs in the future, but I don't think it's too bad right now.

@mewmew
Copy link
Author

mewmew commented Dec 31, 2020

I may be a bit biased because of my day job (lots of Eclipse RCP and SWT work), but I believe that UI widgets are generally more useful if you can still somehow interact with them even though you're not able to modify them. As in the text input example, I think you should still be able to position the cursor, select and copy text. For lists, you should still be able to scroll around and see all the entries, and so on.

After thinking this through, I would agree with you that having read access to disabled widgets is most definitely worth it!

In that regard, maybe the "disabled" flag should get a better name, such as "read-only" (not really a good fit for push buttons, though.) In any case, the flag is not supposed to cover event handling at all. As you mentioned, a read-only text input widget still needs to be able to receive events such as mouse clicks so that the text cursor can be positioned.

For wording, disabled is probably fine. I was mostly surprised that you could delete the text from the widget. If it is grayed out, disabled should be synonymous with read-only.

Speaking of which, another surprising aspect of the disabled Text Input widget was that the carret was yellow also in disabled mode. So it felt that you could edit and remove text. Should the carret also be made e.g. gray then the feeling of the widget really being disabled/read-only would be made clear.

I agree that having to check the flag in multiple places may introduce similar bugs in the future, but I don't think it's too bad right now.

Yeah, for the base widgets, such bugs will be uncovered rather quickly so probably not an issue in practice.

P.S. thanks for giving this some serious thought. I appreciate the discussion.

@blizzy78
Copy link
Contributor

For wording, disabled is probably fine. I was mostly surprised that you could delete the text from the widget. If it is grayed out, disabled should be synonymous with read-only.

Right, that was definitely not intended.

Speaking of which, another surprising aspect of the disabled Text Input widget was that the carret was yellow also in disabled mode. So it felt that you could edit and remove text. Should the carret also be made e.g. gray then the feeling of the widget really being disabled/read-only would be made clear.

I'm a bit torn on this. On the one hand, I want to signal that you can still control the cursor. On the other hand, it probably shouldn't be the same bright yellow as in enabled mode. I should probably allow for a separate disabled caret color, and the demo should use the darker yellow as it does in other places.

P.S. thanks for giving this some serious thought. I appreciate the discussion.

Me too! It's always nice to bounce thoughts off each other, so that's really appreciated. I'm also on Gophers Slack in #ebiten, by the way.

@mewmew
Copy link
Author

mewmew commented Dec 31, 2020

Me too! It's always nice to bounce thoughts off each other, so that's really appreciated. I'm also on Gophers Slack in #ebiten, by the way.

Would you happen to have Discord by chance? :)

@blizzy78
Copy link
Contributor

Would you happen to have Discord by chance? :)

I do, I hang out in Discord Gophers, or you can PM me at blizzy#3493 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants