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

avm1,avm2: Implement TextField.restrict #14634

Merged
merged 5 commits into from
Jan 11, 2024
Merged

Conversation

kjarosh
Copy link
Member

@kjarosh kjarosh commented Jan 6, 2024

I implemented TextField.restrict for both AVM1 and AVM2 (with minor differences). I also added extensive testing to make sure how the restrict actually works, because the documentation is very vague (and wrong in one case).

Currently restrict in Ruffle works only when inputting characters one by one, not when pasting a string (flashplayer does not differentiate between inputting and pasting characters in this case). I'm working on a second PR which will add support for restrict when pasting (it requires more complex testing). Support for restrict when pasting is included.

I guess it's a step towards completing #280.

If I see correctly it's also related to #12613.

@kjarosh
Copy link
Member Author

kjarosh commented Jan 9, 2024

Updated the branch and included the support for pasting (turned out it's not a lot of code). Please let me know if there's anything to improve.

Copy link
Member

@Aaron1011 Aaron1011 left a comment

Choose a reason for hiding this comment

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

Thank you for writing such comprehensive tests!

@torokati44
Copy link
Member

torokati44 commented Jan 11, 2024

This does a fair bit of stuff, including extending our testing capabilities, adding tests (for both of which, thank you very much!), adding a new type, implementing both AVM1 and AVM2 properties - all in a single commit.

Maybe it would be better to split it into a couple logically separate commits that build on each other...?

@kjarosh kjarosh force-pushed the restrict branch 3 times, most recently from 69aab69 to e17fda9 Compare January 11, 2024 12:53
@kjarosh
Copy link
Member Author

kjarosh commented Jan 11, 2024

Thanks for the review!

I've split the god commit into multiple ones. I hope it's now structured in a better way, if you have any suggestions related to the structure of commits please do not hesitate.

Copy link
Contributor

@Dinnerbone Dinnerbone left a comment

Choose a reason for hiding this comment

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

Thanks so much, especially for the tests!

Comment on lines +2454 to +2466
// Caret according to the documentation indicates
// that we are now disallowing characters.
// In reality it just switches allowing/disallowing.
now_allowing = !now_allowing;
Copy link
Member

Choose a reason for hiding this comment

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

lol, nice misdocumentation, and great that you discovered it
(just babbling, no action necessary)

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH it looks like an implementation detail leak. Someone at Macromedia did not think more than one caret may be included in restrict 😃

Copy link
Member

Choose a reason for hiding this comment

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

Yep. And we have seen a lot of these kinds of leaks.

Copy link
Member

Choose a reason for hiding this comment

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

So, thanks, Hyrum's Law...
https://www.hyrumslaw.com/

Comment on lines +2503 to +2517
for interval in intervals {
if self.interval_contains(character, interval) {
return true;
}
}
false
Copy link
Member

Choose a reason for hiding this comment

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

I guess this could have been a call to any, but this isn't any less readable, and no need to delay further just for this.

EditTextRestrict models the `restrict` property, which is used
to specify which characters the user is allowed to type.
Currently in tests (input.json) it is possible to trigger Ctrl-V using:

    { "type": "TextControl", "code": "Paste" },

But there is no way of populating the clipboard.
This patch adds AutomatedEvent::SetClipboardText, so the clipboard
may be populated before pasting:

    { "type": "SetClipboardText", "text": "<value>" },
    { "type": "TextControl", "code": "Paste" },
@Dinnerbone Dinnerbone merged commit 7048646 into ruffle-rs:master Jan 11, 2024
13 checks passed
@kjarosh kjarosh deleted the restrict branch January 11, 2024 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants