Layer-shell: don't drop focus when clicking layer-shell #770
Conversation
Fixes pop-os#769 A layer shell requesting "KeyboardInteractive=False" would cause the compositor to momentarily drop focus from the original surface, and assume no focus. If the layer shell click produces a text input event, then the original surface can't receive the event, breaking the input method.
| under = Some(layer.clone().into()); | ||
| under = Change(layer.clone().into()); | ||
| } else { | ||
| under = Remove; |
There was a problem hiding this comment.
Honestly I think this case should be KeepOld as well. It is probably highly confusing for users for the full-screen app to loose keyboard focus, even if they clicked on an Overlay layer surface.
| target = Change(layer.clone().into()); | ||
| true | ||
| } else { | ||
| target = Remove; |
There was a problem hiding this comment.
I think this is somewhat confusing to read. We set the target to Remove implying the focus to be taken away, but since we return false, we never really evaluate target.
Can we please just make target mut and default initialize it with Remove and then skip these lines? (same goes for line 873)
There was a problem hiding this comment.
Tbh I found the entire function confusing. It's not explained what the "done" is and why it's necessary. Without knowing that, I settled on doing a mechanical translation.
I decided to drop the "mut" because it makes the choices implicit when they are not altered, making the purpose of each branch harder to figure out (need to backtrack).
I think the it can be structured better, rather than fal but I'd need a deeper explanation.
There was a problem hiding this comment.
Fair.
I don't think done is used to great extent any more and is mostly an artifact of previous code. It was introduced as part of this commit to break apart a never ending cascade of if-else statements to figure out the focus target. The previous approach had the problem of never continuing down the tree, if any layer-surface was in the way, no matter if it actually had an input_region there or even took keyboard-focus.
I decided to drop the "mut" because it makes the choices implicit when they are not altered, making the purpose of each branch harder to figure out (need to backtrack).
Also fair, but in that case I feel like we might want to combine done and target (possibly also under), as the state is far too spread out at the moment imo.
I think the it can be structured better, rather than fal but I'd need a deeper explanation.
I hope that is helpful, do you have any further questions? Do you feel like tackling this or would you prefer me doing that?
There was a problem hiding this comment.
I can do it, but I'm away for a week soon.
There was a problem hiding this comment.
I can do it, but I'm away for a week soon.
No rush. I'll ping you, if I happen to find some time to look into this earlier than you do.
Fixes #769
A layer shell requesting "KeyboardInteractive=False" would cause the compositor to momentarily drop focus from the original surface, and assume no focus. If the layer shell click produces a text input event, then the original surface can't receive the event, breaking the input method.
I'm not entirely sure if the change I made makes sense. It's not clear to why anything needs to be done if "done==false".
The overarching question is: what should happen when a layer surface is clicked but doesn't accept focus?
The answer so far was "remove keyboard focus from all applications". I turned it into "keep focus where it was". Considering that the focus jumped immediately back to the same application (but why?), I think this is the right direction.