-
Notifications
You must be signed in to change notification settings - Fork 98
Redispatch unconsumed mouse wheel events #2425
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
Conversation
9a3021d
to
06376d4
Compare
/** | ||
* Returns the first heavyweight ancestor of the given component. | ||
*/ | ||
private fun Component.heavyWeightAncestorOrNull() : Component? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: it could be a top-level function
ea05b5a
to
ee607e3
Compare
Adding a second reviewer, as it is a new API (even as a flag) (UPDATED: potential API via |
* This flag will be removed in the future, and the default behavior will correspond to a value | ||
* of `true`. | ||
*/ | ||
val propagateUnconsumedMouseWheelEvents = FeatureFlag { true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How it's supposed to be changed outside? I guess it should be System.getProperty
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I forgot FeatureFlag
was not mutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer I made FeatureFlag
mutable (and public) or just add a system property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a system property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is enttirely internal
. No, we don't need it in public
ee607e3
to
dc1dac0
Compare
compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/ComposeFeatureFlags.desktop.kt
Outdated
Show resolved
Hide resolved
dc1dac0
to
6219dbb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blockers from me. Please wait @igordmn approval
component.addFocusListener(focusListener) | ||
component.addKeyListener(keyListener) | ||
component.subscribeToMouseEvents(mouseListener) | ||
private fun subscribeToInputEvents() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curiosity, what was wrong with previous shape here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean that it used to take the component to register with as a receiver?
It wasn't needed and was error prone, as the component has to be the same one in all cases.
* of `true`. | ||
*/ | ||
val propagateUnconsumedMouseWheelEvents = FeatureFlag { | ||
System.getProperty("compose.swing.propagateMouseWheelEvents", "true").toBoolean() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this property to the release notes.
Also if we switch it to false in 1.9.1, we have to change the release notes in the cherry-pick.
6219dbb
to
46f7c8f
Compare
This is a cherry-pick (with some adaptations) of #2425 Redispatch mouse wheel events which the Compose scene did not consume. ### Difference with Swing behavior This fix introduces a slight difference with how Swing handles nested mouse-wheel scrolling. In Swing, there is essentially no nested scrolling. In a scenario with a `JScrollPane` inside another `JScrollPane`, only the inner one can be scrolled when the mouse is over it. With this PR, a `ComposePanel` with a scrollable element within a `JScrollPane` will scroll until it reaches the start/end and then the outer `JScrollPane` will scroll. This behavior seems to be an improvement, so we thought it was ok to have this difference. ### Danger, Will Robinson Note that this includes a potentially dangerous hack of whose consequences I'm not yet sure. If it proves too problematic, we will need to take a different approach (documented in the source). The hack is that in order to allow AWT to find the correct target for the redispatched event, we temporarily unregister `ComposeSceneMediator`'s mouse listeners. ### Feature flag I've put the behavior behind `ComposeFeatureFlags.redispatchUnconsumedMouseWheelEvents`. The flag will be: - `false` by default in 1.9.1 - `true` by default in 1.10 - Removed in 1.11, if no major problems are discovered. In this PR, it's `true`, as it merges into `jb-main`. In the cherry-pick to 1.9.1, we should change it to `false`. When it's approved I will add a ticket to remove it for 1.11 ### Other mouse events Note that this PR only deals with mouse wheel events, but at least in the case of a lightweight skia layer (i.e. `SkiaSwingLayer`), we should probably do the same for other mouse events. Fixes https://youtrack.jetbrains.com/issue/CMP-4601 ## Testing Added unit tests, and also tested manually with ``` import androidx.compose.foundation.* import androidx.compose.foundation.layout.* import androidx.compose.material.* import androidx.compose.runtime.Composable import androidx.compose.ui.* import androidx.compose.ui.awt.* import androidx.compose.ui.graphics.* import androidx.compose.ui.unit.* import java.awt.* import javax.swing.* fun main() = SwingUtilities.invokeLater { val frame = JFrame("CfD repro") frame.defaultCloseOperation = JFrame.EXIT_ON_CLOSE frame.minimumSize = Dimension(500, 400) System.setProperty("compose.swing.render.on.graphics", "true") val mainPanel = JPanel(null).apply { val container = JPanel(null).apply { layout = BoxLayout(this, BoxLayout.Y_AXIS) addChildren() } val scrollPane = JScrollPane(container) scrollPane.setBounds(0, 0, 500, 500) add(scrollPane) } frame.contentPane.add(mainPanel, BorderLayout.CENTER) frame.isVisible = true } private fun JPanel.addChildren() { repeat(10) { repeat(5) { add(JLabel("<html>" + "Very long text here ".repeat(10) + "</html>")) add(Box.createVerticalStrut(10)) } add( ComposePanel().apply { setContent { ComposeBox() } } ) add(Box.createVerticalStrut(10)) } } @composable fun ComposeBox() { Box( modifier = Modifier .height(100.dp) .background(color = Color(180, 180, 180)) .padding(10.dp) ) { val stateVertical = rememberScrollState(0) Box( modifier = Modifier .verticalScroll(stateVertical) .padding(end = 12.dp, bottom = 12.dp) ) { Column { for (item in 0..10) { TextBox("Item #$item") } } } } } @composable fun TextBox(text: String = "Item") { Box( modifier = Modifier.height(32.dp) .width(400.dp) .background(color = Color(0, 0, 0, 20)) .padding(start = 10.dp), contentAlignment = Alignment.CenterStart ) { Text(text = text) } } ``` This should be tested by QA ## Release Notes ### Fixes - Desktop - ComposePanel can now re-dispatch unconsumed mouse wheel events, allowing scrollable components beneath to be scrolled. To enable this behavior, set the system property `"compose.swing.redispatchMouseWheelEvents"` to `"true"`.
Redispatch mouse wheel events which the Compose scene did not consume.
Difference with Swing behavior
This fix introduces a slight difference with how Swing handles nested mouse-wheel scrolling. In Swing, there is essentially no nested scrolling. In a scenario with a
JScrollPane
inside anotherJScrollPane
, only the inner one can be scrolled when the mouse is over it. With this PR, aComposePanel
with a scrollable element within aJScrollPane
will scroll until it reaches the start/end and then the outerJScrollPane
will scroll.This behavior seems to be an improvement, so we thought it was ok to have this difference.
Danger, Will Robinson
Note that this includes a potentially dangerous hack of whose consequences I'm not yet sure. If it proves too problematic, we will need to take a different approach (documented in the source).
The hack is that in order to allow AWT to find the correct target for the redispatched event, we temporarily unregister
ComposeSceneMediator
's mouse listeners.Feature flag
I've put the behavior behind
ComposeFeatureFlags.redispatchUnconsumedMouseWheelEvents
. The flag will be:false
by default in 1.9.1true
by default in 1.10In this PR, it's
true
, as it merges intojb-main
. In the cherry-pick to 1.9.1, we should change it tofalse
. When it's approved I will add a ticket to remove it for 1.11Other mouse events
Note that this PR only deals with mouse wheel events, but at least in the case of a lightweight skia layer (i.e.
SkiaSwingLayer
), we should probably do the same for other mouse events.Fixes https://youtrack.jetbrains.com/issue/CMP-4601
Testing
Added unit tests, and also tested manually with
This should be tested by QA
Release Notes
Fixes - Desktop
"compose.swing.redispatchMouseWheelEvents"
to"false"
.