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

Desktop. Fix input methods on JBR, disable input methods when we lose focus #881

Merged
merged 1 commit into from Oct 20, 2023

Conversation

igordmn
Copy link
Collaborator

@igordmn igordmn commented Oct 19, 2023

Rerequest focus on main component when we need to type using input methods

Fixes JetBrains/compose-multiplatform#2628

The issue was because of 2 things:

  • we used a hack to force a focused event (component.inputContext.dispatchEvent(focusGainedEvent)
  • JBR added optimization to ignore focus on the same element (thanks @AiHMin for investigation here). In Compose we have only one element.
    Because optimization is correct, and the hack depends on the internals, it isn't right to fix it in JBR, we should fix it in Compose. Furthermore, even without JBR changes, this hack didn't complete work - we can't for example use it for disabling input methods (see the next point).

In this PR we also use a hack unfortenutely - we refocus the root component, focusing on invisible component first. That leads to another issue with acccessibility, but we fix it here).

A proper fix should be switching to native code, or making an API in JBR (but other vendors still be unsupported).

Don't show input methods popup if there is no focused TextField

Previously we showed a popup, even if there are no focused textfield:
image
Now we don't show it if we isn't in a textfield.
P.S. only on Windows/Linux for now, on macOs Swing seems has a bug

Testing

Tested manually on:

  1. Windows, Chinese/Korean/Japanese, OpenJDK/JBR 17, Accessibility enabled/disabled, ComposeWindow/ComposePanel
  2. macOs, Chinese, OpenJDK/JBR 17, Accessibility enabled/disabled
  3. Linux, Chinese layout, OpenJDK 17

@igordmn igordmn marked this pull request as draft October 19, 2023 11:34
igordmn added a commit that referenced this pull request Oct 19, 2023
1. Panel unavailable on Windows at start
2. When focus on Switch or Box(Modifier.clickable), there was "Panel0". Now it say nothing
3. Fix pronouncing the window title on losing focus in TextField after merging #881 (we refocus main window there)

## Testing
Manually with accessibility enabled on run1 (Windows, macOs)
@igordmn igordmn force-pushed the igor.demin/fix-inputmethods-with-jbr-2 branch from 7a4d1b2 to 9751952 Compare October 19, 2023 22:54
igordmn added a commit that referenced this pull request Oct 19, 2023
1. Windows pronounced "Panel unavailable" at start
2. When we focus on Switch or Box(Modifier.clickable), there was "Panel0" pronounced. Now it says nothing.
3. After merging #881 refocus the main component. This leads to pronouncing the window title, now it says nothing, as the accessible root node is unknown

## Testing
Manually with accessibility enabled on run1 (Windows, macOs)
@igordmn igordmn force-pushed the igor.demin/fix-inputmethods-with-jbr-2 branch from 9751952 to c85c06b Compare October 19, 2023 22:59
1. Fixes JetBrains/compose-multiplatform#2628
2. Doesn't show input methods popup if there is no focused TextField (only on Windows for now, on macOs, Swing seems has a bug: JetBrains/compose-multiplatform#3839)

## Testing
Tested manually on Windows/macOs/Linux, OpenJDK/JBR, Accessibility enabled/disabled
igordmn added a commit that referenced this pull request Oct 19, 2023
…ixes issues on Windows)

This fixes issues on Windows:

1. Windows pronounced "Panel unavailable" at start
2. When we focus on Switch or Box(Modifier.clickable), there was "Panel0" pronounced. Now it says nothing.
3. After merging #881 refocus the main component. This leads to pronouncing the window title, now it says nothing, as the accessible root node is unknown

on macOs it doesn't change anything

## Testing
Manually with accessibility enabled on run1 (Windows, macOs)
@igordmn igordmn force-pushed the igor.demin/fix-inputmethods-with-jbr-2 branch from c85c06b to 04f68f5 Compare October 19, 2023 23:52
@igordmn igordmn changed the title Desktop. Fix input methods Desktop. Fix Input methods on JBR, disable input methods when we lose focus Oct 20, 2023
@igordmn igordmn changed the title Desktop. Fix Input methods on JBR, disable input methods when we lose focus Desktop. Fix input methods on JBR, disable input methods when we lose focus Oct 20, 2023
@igordmn igordmn force-pushed the igor.demin/fix-inputmethods-with-jbr-2 branch 3 times, most recently from a435648 to 7ebb7dc Compare October 20, 2023 01:11
@igordmn igordmn marked this pull request as ready for review October 20, 2023 01:12
@igordmn igordmn requested a review from Walingar October 20, 2023 01:13
private val platformComponent: PlatformComponent = object : PlatformComponent {
override fun enableInput(inputMethodRequests: InputMethodRequests) {
currentInputMethodRequests = inputMethodRequests
component.enableInputMethods(true)
val focusGainedEvent = FocusEvent(focusComponentDelegate, FocusEvent.FOCUS_GAINED)
component.inputContext.dispatchEvent(focusGainedEvent)
// Without resetting the focus, Swing won't update the status (doesn't show/hide popup)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to cover such a case with a test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There 2 useful tests that could be written:

  1. Checking that some Swing component enables input methods when we focus on a textfield. Unfortunately, Swing doesn't provide any API for that.

  2. Checking Chinese input in integration with Chinise input enabled in the system. But for that we need a Docker container, a task on CI, and a special test annotation @environmenttest.

Other kind of tests aren't useful:

  1. Checking that TextField enables Input methods on ComposeScene level. It isn't very needed right now, as a similar test already exists in Jetpack Compose (that checks that we call appropriate PlatformTextInputService methods).
  2. Injecting fake JComponent, on which we check that we call enableInputMethods. But this bounds the test to the implementation details.

Copy link
Collaborator

@Walingar Walingar left a comment

Choose a reason for hiding this comment

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

Since this is kinda a hack, we need to discuss better solution with JBR team
I am pretty sure that it can shoot somewhere..
But I am ok to have it for now

abstract val component: JComponent
val invisibleComponent: Component get() = _invisibleComponent
Copy link
Collaborator

Choose a reason for hiding this comment

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

internal

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The class ComposeBridge is internal, so no need to add it for field

@@ -187,6 +188,7 @@ class ComposePanel @ExperimentalComposeUiApi constructor(
if (bridge == null) {
bridge = createComposeBridge()
initContent()
super.add(bridge!!.invisibleComponent, Integer.valueOf(1))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we add it to another layer then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will it make a difference? super.add(, 0) looks the same to me as when we just add it before the other component.

@igordmn
Copy link
Collaborator Author

igordmn commented Oct 20, 2023

Since this is kinda a hack, we need to discuss better solution with JBR team

Created https://youtrack.jetbrains.com/issue/COMPOSE-520/Desktop.-Rewrite-input-methods

igordmn added a commit that referenced this pull request Oct 20, 2023
… issues on Windows) (#885)

This fixes issues on Windows:

1. Windows pronounced "Panel unavailable" at start
2. When we focus on Switch or Box(Modifier.clickable), there was
"Panel0" pronounced. Now it says nothing.
3. After merging
#881, we
refocus the main component. This leads to pronouncing the window title,
now it says nothing, as the accessible root node is unknown

on macOs it changes one thing:
1. When we focus on Switch or Box(Modifier.clickable), now it prounonces
that we in scroll area (if we in scroll area). It also pronounces it for
other compnents like Button/Text

## Testing
Manually with accessibility enabled in run1 (Windows, macOs)
@igordmn igordmn merged commit dd86b0b into jb-main Oct 20, 2023
3 checks passed
@igordmn igordmn deleted the igor.demin/fix-inputmethods-with-jbr-2 branch October 20, 2023 18:39
igordmn added a commit that referenced this pull request Oct 20, 2023
… issues on Windows) (#885)

This fixes issues on Windows:

1. Windows pronounced "Panel unavailable" at start
2. When we focus on Switch or Box(Modifier.clickable), there was
"Panel0" pronounced. Now it says nothing.
3. After merging
#881, we
refocus the main component. This leads to pronouncing the window title,
now it says nothing, as the accessible root node is unknown

on macOs it changes one thing:
1. When we focus on Switch or Box(Modifier.clickable), now it prounonces
that we in scroll area (if we in scroll area). It also pronounces it for
other compnents like Button/Text

## Testing
Manually with accessibility enabled in run1 (Windows, macOs)
igordmn added a commit that referenced this pull request Oct 20, 2023
… focus (#881)

### Rerequest focus on main component when we need to type using input
methods
Fixes JetBrains/compose-multiplatform#2628

The issue was because of 2 things:
- we used a hack to force a focused event
(`component.inputContext.dispatchEvent(focusGainedEvent)`
- JBR added optimization to ignore focus on the same element (thanks
@AiHMin for investigation
[here](JetBrains/compose-multiplatform#2628 (comment))).
In Compose we have only one element.
Because optimization is correct, and the hack depends on the internals,
it isn't right to fix it in JBR, we should fix it in Compose.
Furthermore, even without JBR changes, this hack didn't complete work -
we can't for example use it for disabling input methods (see the next
point).

In this PR we also use a hack unfortenutely - we refocus the root
component, focusing on invisible component first. That leads to another
issue with acccessibility, but we fix it
[here](#885)).

A proper fix should be switching to native code, or making an API in JBR
(but other vendors still be unsupported).

### Don't show input methods popup if there is no focused TextField 

Previously we showed a popup, even if there are no focused textfield:

![image](https://github.com/JetBrains/compose-multiplatform-core/assets/5963351/82e3543f-11f4-4013-8da5-d782b824ed2c)
Now we don't show it if we isn't in a textfield.
P.S. only on Windows/Linux for now, on macOs Swing seems has [a
bug](JetBrains/compose-multiplatform#3839)

## Testing
Tested manually on:
1. Windows, Chinese/Korean/Japanese, OpenJDK/JBR 17, Accessibility
enabled/disabled, ComposeWindow/ComposePanel
2. macOs, Chinese, OpenJDK/JBR 17, Accessibility enabled/disabled
4. Linux, Chinese layout, OpenJDK 17
mazunin-v-jb pushed a commit that referenced this pull request Dec 7, 2023
… issues on Windows) (#885)

This fixes issues on Windows:

1. Windows pronounced "Panel unavailable" at start
2. When we focus on Switch or Box(Modifier.clickable), there was
"Panel0" pronounced. Now it says nothing.
3. After merging
#881, we
refocus the main component. This leads to pronouncing the window title,
now it says nothing, as the accessible root node is unknown

on macOs it changes one thing:
1. When we focus on Switch or Box(Modifier.clickable), now it prounonces
that we in scroll area (if we in scroll area). It also pronounces it for
other compnents like Button/Text

## Testing
Manually with accessibility enabled in run1 (Windows, macOs)
mazunin-v-jb pushed a commit that referenced this pull request Dec 7, 2023
… focus (#881)

### Rerequest focus on main component when we need to type using input
methods
Fixes JetBrains/compose-multiplatform#2628

The issue was because of 2 things:
- we used a hack to force a focused event
(`component.inputContext.dispatchEvent(focusGainedEvent)`
- JBR added optimization to ignore focus on the same element (thanks
@AiHMin for investigation
[here](JetBrains/compose-multiplatform#2628 (comment))).
In Compose we have only one element.
Because optimization is correct, and the hack depends on the internals,
it isn't right to fix it in JBR, we should fix it in Compose.
Furthermore, even without JBR changes, this hack didn't complete work -
we can't for example use it for disabling input methods (see the next
point).

In this PR we also use a hack unfortenutely - we refocus the root
component, focusing on invisible component first. That leads to another
issue with acccessibility, but we fix it
[here](#885)).

A proper fix should be switching to native code, or making an API in JBR
(but other vendors still be unsupported).

### Don't show input methods popup if there is no focused TextField 

Previously we showed a popup, even if there are no focused textfield:

![image](https://github.com/JetBrains/compose-multiplatform-core/assets/5963351/82e3543f-11f4-4013-8da5-d782b824ed2c)
Now we don't show it if we isn't in a textfield.
P.S. only on Windows/Linux for now, on macOs Swing seems has [a
bug](JetBrains/compose-multiplatform#3839)

## Testing
Tested manually on:
1. Windows, Chinese/Korean/Japanese, OpenJDK/JBR 17, Accessibility
enabled/disabled, ComposeWindow/ComposePanel
2. macOs, Chinese, OpenJDK/JBR 17, Accessibility enabled/disabled
4. Linux, Chinese layout, OpenJDK 17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants