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

core: Add basic support for Tab key behavior #15542

Merged
merged 8 commits into from
Mar 26, 2024

Conversation

kjarosh
Copy link
Member

@kjarosh kjarosh commented Mar 12, 2024

Related to #5443. Improves coverage for AVM1: #243, #263, #280.

The Tab key is used to cycle through focusable elements in stage. It supports two tab orderings: automatic and custom. This patch adds basic support for this behavior and both modes.

Things implemented:

  • Tab key now cycles through the tab ordering and changes the focus.
  • Tab key now works on desktop (instead of cycling through interactive GUI elements).
  • Automatic tab ordering.
  • Custom tab ordering.
  • TextField.tabEnabled in AVM1.
  • TextField.tabIndex in AVM1.
  • Button.tabEnabled in AVM1.
  • Button.tabIndex in AVM1.
  • MovieClip.tabEnabled in AVM1.
  • MovieClip.tabIndex in AVM1.

Things known to be missing/broken yet:

  • MovieClip.tabChildren in AVM1 is not implemented.
  • No AVM2 support yet.
  • There's no visual highlight of the tabbed element.
  • The order in automatic tab ordering is not fine-tuned.

..
}
) {
// Prevent egui from consuming the Tab key.
Copy link
Collaborator

Choose a reason for hiding this comment

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

On #13426, it was stated that

The TAB key is used to access the inventory in the game, but in the Ruffle desktop player, TAB moves through Ruffle's own menus and doesn't forward the key to the game. Fortunately, you can also open the inventory by clicking the backpack with the mouse...

Does this change fix that?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is true, it should be fixed now.

@kjarosh kjarosh force-pushed the tab-ordering branch 3 times, most recently from a38d008 to 1ac611b Compare March 14, 2024 21:44
@kjarosh kjarosh force-pushed the tab-ordering branch 2 times, most recently from 2249ad3 to 87bd3c3 Compare March 19, 2024 17:14
@danielhjacobs danielhjacobs added waiting-on-review Waiting on review from a Ruffle team member input Issues relating to user input in Flash content labels Mar 21, 2024
@kjarosh kjarosh force-pushed the tab-ordering branch 2 times, most recently from 5a85439 to 6b1768c Compare March 22, 2024 18:08
Copy link
Collaborator

@adrian17 adrian17 left a comment

Choose a reason for hiding this comment

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

Approved, assuming the nits are fixed and that weird behavior I found on discord is documented (maybe a failing test?)

@@ -46,6 +46,7 @@ pub struct Avm1ButtonData<'gc> {
object: Lock<Option<Object<'gc>>>,
initialized: Cell<bool>,
has_focus: Cell<bool>,
tab_index: Cell<Option<i32>>,
Copy link
Collaborator

@adrian17 adrian17 Mar 25, 2024

Choose a reason for hiding this comment

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

can you document all these places, that they might eventually need to be moved to InteractiveObject?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, added a TODO for each one

/// Used to customize tab ordering.
/// When not `None`, a custom ordering is used, and
/// objects are ordered according to this value.
fn get_tab_index(&self) -> Option<i64> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: as a general convention, getter-like functions don't have the get_ prefix in their name

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, removed the prefix from all getters

self.0.read().tab_index
}

pub fn set_tab_index_value(&self, context: &mut UpdateContext<'_, 'gc>, value: Option<i32>) {
Copy link
Collaborator

@adrian17 adrian17 Mar 25, 2024

Choose a reason for hiding this comment

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

same here, just tab_index and set_tab_index?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the prefix, but left the _value suffix due to the i32/u32 thingy and a potential name conflict with tab_index

@kjarosh kjarosh force-pushed the tab-ordering branch 2 times, most recently from 41853fc to bec2ad4 Compare March 25, 2024 23:31
@kjarosh
Copy link
Member Author

kjarosh commented Mar 25, 2024

Added a test tab_ordering_properties_tab_index_edge_case which documents the weird behavior from Discord.

kjarosh added 8 commits March 27, 2024 00:28
The Tab key is used to cycle through focusable elements in stage.
It supports two tab orderings: automatic and custom.
This patch adds basic support for this behavior.
This test checks the basic support of tab ordering
and its automatic mode.
This test checks the basic support of tab ordering
and its custom mode.
This test verifies the behavior of properties related to tab ordering.
This test covers the edge case of MovieClip.tabIndex,
where setting it to a string at the start returns this string
instead of a numerical value.
@adrian17 adrian17 enabled auto-merge (rebase) March 26, 2024 23:30
@adrian17 adrian17 merged commit 67a04dc into ruffle-rs:master Mar 26, 2024
15 of 16 checks passed
@kjarosh kjarosh deleted the tab-ordering branch March 26, 2024 23:45
@Lord-McSweeney Lord-McSweeney removed the waiting-on-review Waiting on review from a Ruffle team member label Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
input Issues relating to user input in Flash content newsworthy
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants