After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 753371 - Bad focus order in GtkFlowBox
Bad focus order in GtkFlowBox
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 735994
 
 
Reported: 2015-08-07 21:45 UTC by Rafal Luzynski
Modified: 2015-10-04 03:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
An example to test the bug (1.75 KB, text/plain)
2015-08-07 21:45 UTC, Rafal Luzynski
  Details
The simplest patch which (partially) solves the problem (531 bytes, patch)
2015-08-21 10:34 UTC, Rafal Luzynski
none Details | Review
Solves the problem (at least partially) (2.29 KB, patch)
2015-08-23 00:55 UTC, Rafal Luzynski
none Details | Review
Solves the problem (better than before but still partially) (2.68 KB, patch)
2015-09-05 20:41 UTC, Rafal Luzynski
none Details | Review
Seems to solve the problem (6.30 KB, patch)
2015-09-09 23:31 UTC, Rafal Luzynski
committed Details | Review

Description Rafal Luzynski 2015-08-07 21:45:01 UTC
Created attachment 308915 [details]
An example to test the bug

I am going to use GtkFlowBox as a better and more flexible replacement of GtkGrid. I want to add some child widgets to it and I want them to get the focus. I don't want the selection feature and I don't want the box to get the focus for itself. I think it should be possible with selection="none" and accept-focus="false". Unfortunately, it does not work as I expect. With these settings the box does not accept the focus and passes it to the next sibling. As far as I know most containers which don't accept the focus pass it to their children. GtkFlowBox does not pass the focus to its children unless it steals the focus first.

Actually, it's not the GtkFlowBox what steals the focus but its intermediate child GtkFlowBoxChild. I think it should be transparent to the focus, at least sometimes.
Comment 1 Matthias Clasen 2015-08-16 16:10:14 UTC
I've done some experiments with listbox, but it is hard to get the desired behavior without breaking some other forms of directional keynav.
Comment 2 Rafal Luzynski 2015-08-16 21:21:02 UTC
Thank you for your effort, Matthias. If that helps, I'd like to have the desired behavior with can_focus=FALSE. The behavior with can_focus=TRUE should not be changed so nothing should be broken. Currently with can_focus=FALSE the focus order totally skips the GtkFlowBox and all its descendants which I think is not correct.

I am not sure if the same applies to GtkListBox.
Comment 3 Rafal Luzynski 2015-08-21 10:34:58 UTC
Created attachment 309798 [details] [review]
The simplest patch which (partially) solves the problem

This is the patch that partially solves the problem. It is incomplete so please don't commit it, take it as a template for the discussion.

As I wrote above, I think that the current behavior of GtkFlowBox with can-focus=TRUE is correct and I don't want to change it. My desired behavior should be with can-focus=FALSE but it is incorrect now.

Here is what happens. It turns out that both GtkFlowBox and GtkFlowBoxChild ignore the can-focus property completely. GtkFlowBox acts as if it had always can-focus=FALSE and I think it is fine: it never tries to grab the focus for itself and never should. GtkFlowBoxChild acts as if it had always can-focus=TRUE. Even with can-focus=FALSE it tries to grab the focus: gtk_flow_box_update_cursor() calls gtk_widget_grab_focus() which fails silently, or more precisely: does nothing. This is the reason why the focus does not move and stays where it has been when GtkFlowBoxChild with can-focus=FALSE is the next in the focus order. Correctly it should pass the focus to its child immediately. See gtk_container_focus() as a correct example. Note that GtkFlowBoxChild is derived from GtkBin and it has at most one child.

Advantage of this patch: the Tab key works as expected.

What is missing:
- focus movement with the arrow keys does not yet work,
- requires setting can-focus=FALSE for each GtkFlowBoxChild explicitly.

Another problem is how can-focus property is handled. I think that it would be intuitive if setting the can-focus property of GtkFlowBox changed the value of can-focus of all its GtkFlowBoxChildren. I don't think the developers want to manage the can-focus property of each GtkFlowBoxChild explicitly and individually. GtkFlowBoxChild is meant to be transparent and the caller should not care if there is an intermediate child between the GtkFlowBox and the real child or not and whether it has different properties than the box.
Comment 4 Rafal Luzynski 2015-08-23 00:55:59 UTC
Created attachment 309875 [details] [review]
Solves the problem (at least partially)

What's fixed:
* Tab and Shift-Tab work as they should.
* Keynav (arrow keys) move the focus as expected within the flow box.
* Behavior with "can-focus"==TRUE not changed so nothing broken.

What's not fixed:
* Keynav does not work fine with the widgets outside the flow box. I think that when the focus enters the flow box as a result of the arrow key pressed then it should select the child which is nearest the previously focused widget, like GtkGrid and GtkBox do. Now it selects the first widget. Similar problem when the keynav reaches the edge of the flow box: it should move the focus to the nearest widget outside the box. Now it does nothing. But I'm afraid it would break some behavior with "can-focus"==TRUE. Maybe we need a separate flag "act-like-a-grid-not-like-a-list-box" ?
* The problem with setting the "can-focus" property is not fixed and I don't know how to fix it. I think it would be better if setting "can-focus" of the GtkFlowBox also set the value of all its GtkFlowBoxChild intermediate children. Or better all GtkFlowBoxChild children should always have the same value of "can-focus" as their parent. It does not make much sense to have a separate "can-focus" value for each of them, also it may be difficult to understand for the developers why they should enumerate all children of GtkFlowBox and change their "can-focus" value rather than change the value of GtkFlowBox itself.
Comment 5 Matthias Clasen 2015-08-26 23:13:17 UTC
(In reply to Rafal Luzynski from comment #3)

> Another problem is how can-focus property is handled. I think that it would
> be intuitive if setting the can-focus property of GtkFlowBox changed the
> value of can-focus of all its GtkFlowBoxChildren. I don't think the
> developers want to manage the can-focus property of each GtkFlowBoxChild
> explicitly and individually. GtkFlowBoxChild is meant to be transparent and
> the caller should not care if there is an intermediate child between the
> GtkFlowBox and the real child or not and whether it has different properties
> than the box.

I'm afraid it will have to stay this way. can-focus is a property of the individual widget, and should be set on individual widgets, similar to the the selectable and activatable properties we already have on the items. Having can-focus on the box propagate down to the children would be too magic for my taste. If you have to create many items, writing a create_item() function is not exactly hard...
Comment 6 Rafal Luzynski 2015-08-27 20:54:41 UTC
This makes the things easier because I don't have to worry about propagation of can-focus.

Now the question is: when the user navigates the focus into the flow box with the arrow keys how should we know if they want to focus the first/last child (like in a list box) or the child which is nearest the previously focused widget (like GtkBox, GtkGrid etc.?) Similarly if they navigate with the arrow keys inside the box and reach the edge of the flow box, do they want to navigate out and focus the nearest widget outside (like GtkBox and GtkGrid) or beep and stay inside (like a list box or edit boxes) until they press Tab?
Comment 7 Rafal Luzynski 2015-09-05 20:41:59 UTC
Created attachment 310723 [details] [review]
Solves the problem (better than before but still partially)

This patch also solves the problem with the focus entering the flow box from another widget with keynav. Now if "can-focus" of the flow box is FALSE then it passes the focus handling to the default GtkContainer implementation which is the best for this case: it just sets the focus to the child which is the closest to the previous widget. I think that the condition "can-focus"=FALSE is correct, we don't have to worry about "can-focus" of all or some children.

Now the problem with the keynav within the flow box: it works fine with Tab/Shift+Tab and fine with the arrow keys as long as we stay inside the flow box. When it hits the border it beeps while it should pass the focus outside the box. Also the keys Home/End/PgUp/PgDn do work while I think they should not, they should be handled by the parent widget of the box. This behavior is controlled by the key binding which emits the "move-cursor" signal. I'd like the key binding to be temporarily undone while "can-focus"=FALSE but I don't know how to do it. I already did some experiments with the binding code commented out and it works perfectly for me but this is not the correct solution to disable the key binding always.
Comment 8 Rafal Luzynski 2015-09-09 23:31:11 UTC
Created attachment 311028 [details] [review]
Seems to solve the problem

This patch changes the return type of "move-cursor" signal from void to gboolean. Bad news is that it breaks the API. Good news is that hopefully nobody has yet used this API, the API was probably incorrect if it caused the problems, and that now it works.

I have also removed the sentence "If the cursor is not visible in @text_view, this signal causes the viewport to be moved instead." from the doc of this signal because it is probably a direct copy from GtkTextView and it is false here.