Bug 336720 - Implementation change in ScrollArea broke keyboard focus handling (plasma-framework.git 66e2b91)
Summary: Implementation change in ScrollArea broke keyboard focus handling (plasma-fra...
Status: RESOLVED FIXED
Alias: None
Product: libplasma
Classification: Frameworks and Libraries
Component: components (other bugs)
Version First Reported In: unspecified
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: Marco Martin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-25 18:31 UTC by Eike Hein
Modified: 2016-05-04 12:13 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed/Implemented In:
Sentry Crash Report:


Attachments
Testcase (622 bytes, application/octet-stream)
2014-06-25 19:18 UTC, Eike Hein
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eike Hein 2014-06-25 18:31:55 UTC
66e2b91 in plasma-framework - the change of ScrollArea to use QQC ScrollView internally - broke existing code with regard to keyboard focus handling.

Folder has a FocusScope { MouseEventListener { ScrollArea { GridView {} } } }  hierarchy, and with both the GridView and the FocusScope having focus, keyboard events aren't delivered to the GridView anymore.

This works again when reverting 66e2b91.

Reproducible: Always
Comment 1 Eike Hein 2014-06-25 18:32:37 UTC
CC'ing David who authored the commit, Bushan who reproduced it and Elias who confirmed reverting fixes it.
Comment 2 Eike Hein 2014-06-25 18:45:07 UTC
https://bugreports.qt-project.org/browse/QTBUG-31976 might be relevant, as flicking indeed is turned off now too.
Comment 3 Eike Hein 2014-06-25 18:51:19 UTC
The workaround suggested in the Qt bug doesn't work for me, though.

I think we should consider reverting 66e2b91; even aside from the keyboard breakage for ListView/GridView I don't think we want to disable flicking by default in a low-level component like ScrollArea that's also used by Plasma UIs on touch devices.
Comment 4 Eike Hein 2014-06-25 18:53:21 UTC
(And yeah, I'm aware of the consequences for scrollbars - the KDE 4 version of Kicker had probably the only dynamically filtered list views with working scrollbars in Plasma 1, since I switched to positioning items myself on the contentItem to make the math work :/. At least QtQuick 2 has originX/Y now.)
Comment 5 Eike Hein 2014-06-25 18:54:46 UTC
I can confirm that this also breaks keyboard navigation in Kicker (ListViews in ScrollArea).
Comment 6 Eike Hein 2014-06-25 19:18:06 UTC
Created attachment 87402 [details]
Testcase

Attachment is a reduced testcase. Run with qmlscene. Arrow down should switch items (and does if you revert).
Comment 7 Eike Hein 2014-06-25 19:47:07 UTC
Unfortunately explicitly setting 'interactive' to true in our wrapper around ScrollView wouldn't be a good fix either, since setting it one way or the other still takes control away from applet authors without hacks on their end and there are use cases for both modes.

Looks like we might need to fork ScrollView for now to remove the 'interactive' override, until the problem can be addressed somehow in 5.4. Unfortunately even if 'interactive' is split up into two props for mouse and keyboard, fixing the keyboard problem, disabling flicking by default is still not what we'd want to do, so we need to talk to upstream about this somehow.
Comment 8 David Edmundson 2014-06-25 20:34:54 UTC
>disabling flicking by default

In QtQuickControls interactive is disabled depending on whether we have a touch screen or not, which sounds like exactly what we want in Plasma. So if upstream does split interactive we should be ok? Unfortunately that'll be 5.4 material regardless.

I can confirm removing the interactive: false does fix everything;  proving the contentItem has "focus: true" *and* the scrollview does too. ScrollView acts as a focus scope.

The focus being needed on the ScrollView makes sense, the focus true on the contentItem is something that could/should be done in Qt.

Our options are: 
 1) Fork ScrollView.qml and stop it changing  the interactive property (it's just one QML file) until 5.4

 2) In our ScrollArea have a onContentItemChanged :{ contentItem.interactive = true}. 
As Eike says, this does break applets being able to set interactive = false themselves, but is that a real problem?

 3) Revert this change
 (I'd rather not.. it fixes other things, but it's good that we have a backup plan.)

 4) Implement Keys.onLeftPressed in our ScrollView subclass until 5.4 and manually incrementIndex() / decrementIndex() if the contentItem is a list/grid. 
Has most the same downsides as option 2, but the flicking-ness will still only be touch screen only, which some applet developers seemed to want.
Comment 9 Eike Hein 2014-06-25 20:52:22 UTC
> So if upstream does split interactive we should be ok?

Yep, guess so ... flicking with no touch input device is probably not needed.

> As Eike says, this does break applets being able to set interactive = false themselves, but is that a real problem?

We'd have to check whether we currently have any code that cares I guess, but it's certainly technically a bug when you can't simply set a prop declaratively anymore without insight into the implementation. Some views might want to intentionally disable flicking or keyboard input ... I can't immediately think of a real use case either though ...

>  3) Revert this change
> (I'd rather not.

Ditto, it'd get us further away from the ideal (ScrollView working for us) and creates new work (fixing the scrollbars).

> Has most the same downsides as option 2, but the flicking-ness will still only be touch screen only, which some applet developers seemed to want.

That doesn't really work, we have complex users that reimplement arrow key handling. For example, Folder implements a complex item view with Ctrl/Shift modifier handling (selection extending, anchored selections, etc.) and also has to reimplement spatial keyboard navigation (to jump across gaps in desktop icons). We can't take over keyboard handling usefully.
Comment 10 Bhushan Shah 2014-06-26 03:39:21 UTC
One more bug reported on forum, related maybe? http://forum.kde.org/viewtopic.php?f=287&t=121740
Comment 11 Marco Martin 2014-09-26 11:53:02 UTC
I think interactive should be enabled regardless, and iinteractive items eventually have preventStealing set to favor drag managed by items vs managed by the scrollarea.

the problem if forums should be unrelated.
Comment 12 Marco Martin 2016-05-04 12:13:10 UTC
since a while interactive is actually based on the "ismobile" property in internal qqc settings (that will be true only in android, plasma mobile and what not)