Created attachment 326220 [details] [review]
[PATCH 1/2] canvas-item: don't use atk_focus_tracker_notify()

Created attachment 326221 [details] [review]
[PATCH 2/2] canvas-container: don't use atk_focus_tracker_notify()

Review of attachment 326220 [details] [review]:

::: libnautilus-private/nautilus-canvas-item.c
@@ +341,2 @@
 		if (details->is_highlighted_as_keyboard_focus) {
+			atk_object_notify_state_change (accessible, ATK_STATE_FOCUSED, TRUE);

I don't think this is equivalent. You now want to specify the state directly... but you are doing it only for the TRUE state.
Look how is done for PROP_HIGHLIGHTED_FOR_SELECTION

Review of attachment 326221 [details] [review]:

I thinks this one looks good, thanks!

In any case worth to check with people from accessibility. Could you join their channel #a11y on irc.gnome.org and ask them?

Okay, so the deprecated stuff runs a little deeper, namely, the overriding of add_focus_handler() and remove_focus_handler() (https://git.gnome.org/browse/nautilus/tree/eel/eel-canvas.c#n3637).
Connecting to the “state-change” signal is recommended for tracking, instead of using atk_component_{add,remove}_focus_handler().

Not entirely sure what to make of this.

Created attachment 326673 [details] [review]
canvas-item: use atk_object_notify_state_change()

atk_focus_tracker_notify() has been deprecated since ATK 2.9.4 and using
atk_object_notify_state_change() is recommended.

This commit replaces the call to the deprecated function.

Created attachment 326674 [details] [review]
canvas-item: use atk_component_get_extents()

atk_component_get_position() has been deprecated since ATK 2.12 and
using atk_component_get_extents() is recommended.

This commit replaces calls to the deprecated function.

Created attachment 326675 [details] [review]
canvas-container: don't fire state-changed:focused on mouse clicks

ATK_STATE_FOCUSED indicates keyboard focus and is set erroneously on
mouse clicks. In some cases it is done twice and, since
set_keyboard_focus() already indirectly sets the state, doing so is
redundant.

This commit removes calls to emit_atk_focus_tracker_notify() in
inappropriate places.

Created attachment 326676 [details] [review]
canvas-container: stop using atk_focus_tracker_notify()

atk_focus_tracker_notify() has been deprecated since ATK 2.9.4 and using
atk_object_notify_state_change() is recommended.

This commit replaces the deprecated function call.

Created attachment 326677 [details] [review]
canvas-container: use clear_keyboard_rubberband_start()

The keyboard rubberband start variable is cleared inconsistently.

This commit replaces the occurence of direct manipulation with a
function call.

Created attachment 326678 [details] [review]
eel-canvas: don't override deprecated functions

AtkComponentIface's {add,remove}_focus_handler should not be overridden,
since ATK 2.9.4, as atk_component_{add,remove}_focus_handler() are
deprecated.

This commit removes deprecated virtual function overrides.

Review of attachment 326673 [details] [review]:

Makes sense

Review of attachment 326674 [details] [review]:

A different approach would be to implement something similar as atk, and merge the position and size handling together.
But seems we do something really specific towards size, so I think this patch is good to go as it is and further improvement doesn't worth the effort given that this will only be use by the desktop soon.
so LGTM

Review of attachment 326675 [details] [review]:

I'm not sure why is wrong to set it on mouse clicks, the items get selected no and that means focus for atk right?

Review of attachment 326676 [details] [review]:

::: libnautilus-private/nautilus-canvas-container.c
@@ +3338,3 @@
 		
+		emit_atk_object_notify_focused (icon);
+

why is this necessary here now but didn't before? Is because of the previous patch that removed a needed atk_notify?

Review of attachment 326677 [details] [review]:

looks good thanks!

Review of attachment 326678 [details] [review]:

This one only is fine if the previous ones are also committed right? In any case, looks good once that's the case. Marking as accepted commit now, but only commit when the others are.

(In reply to Carlos Soriano from comment #15)
> Review of attachment 326675 [details] [review] [review]:
> 
> I'm not sure why is wrong to set it on mouse clicks, the items get selected
> no and that means focus for atk right?

state-changed:selected and selection-changed are fired on mouse selection (as with keyboard navigation). The items are still read when clicked, just not when clicked with a modifier (didn’t work with ctrl anyway and without the deprecated atk_focus_tracker_notify(), shift-selection reading stops working properly). The emphasis is put on the keyboard with ATK_STATE_FOCUSED, so I decided to focus (pardon the pun) on the keyboard.

There’s nothing wrong with it per se.

I can consult with #a11y on this one.

(In reply to Carlos Soriano from comment #16)
> Review of attachment 326676 [details] [review] [review]:
> 
> ::: libnautilus-private/nautilus-canvas-container.c
> @@ +3338,3 @@
>  		
> +		emit_atk_object_notify_focused (icon);
> +
> 
> why is this necessary here now but didn't before? Is because of the previous
> patch that removed a needed atk_notify?

Oops, my bad.
Yeah, it’s because I removed the calls in a previous patch.

The purpose of it is so the signal gets emitted, but the focus isn’t drawn.
That’s the way it has been for a while, so I didn’t change the behavior.

(In reply to Ernestas Kulik from comment #19)
> (In reply to Carlos Soriano from comment #15)
> > Review of attachment 326675 [details] [review] [review] [review]:
> > 
> > I'm not sure why is wrong to set it on mouse clicks, the items get selected
> > no and that means focus for atk right?
> 
> state-changed:selected and selection-changed are fired on mouse selection
> (as with keyboard navigation). The items are still read when clicked, just

Why is relevant that those signals are emitted? Also, what do you mean with "still read"?

> not when clicked with a modifier (didn’t work with ctrl anyway and without
> the deprecated atk_focus_tracker_notify(), shift-selection reading stops
> working properly). The emphasis is put on the keyboard with
> ATK_STATE_FOCUSED, so I decided to focus (pardon the pun) on the keyboard.

You mean with the patch the shift modifier stop working to select the items?

> 
> There’s nothing wrong with it per se.
> 
> I can consult with #a11y on this one.

I think the problem is me not understanding it more than the patch being not correct or so :)

(In reply to Ernestas Kulik from comment #20)
> (In reply to Carlos Soriano from comment #16)
> > Review of attachment 326676 [details] [review] [review] [review]:
> > 
> > ::: libnautilus-private/nautilus-canvas-container.c
> > @@ +3338,3 @@
> >  		
> > +		emit_atk_object_notify_focused (icon);
> > +
> > 
> > why is this necessary here now but didn't before? Is because of the previous
> > patch that removed a needed atk_notify?
> 
> Oops, my bad.
> Yeah, it’s because I removed the calls in a previous patch.
> 
> The purpose of it is so the signal gets emitted, but the focus isn’t drawn.
> That’s the way it has been for a while, so I didn’t change the behavior.

Ah ok, move the code to the patch it belongs and I think it will be fine.

(In reply to Carlos Soriano from comment #21)
> (In reply to Ernestas Kulik from comment #19)
> > (In reply to Carlos Soriano from comment #15)
> > > Review of attachment 326675 [details] [review] [review] [review] [review]:
> > > 
> > > I'm not sure why is wrong to set it on mouse clicks, the items get selected
> > > no and that means focus for atk right?
> > 
> > state-changed:selected and selection-changed are fired on mouse selection
> > (as with keyboard navigation). The items are still read when clicked, just
> 
> Why is relevant that those signals are emitted? Also, what do you mean with
> "still read"?
> 

Ah, yes, I’m talking about Orca.

> > not when clicked with a modifier (didn’t work with ctrl anyway and without
> > the deprecated atk_focus_tracker_notify(), shift-selection reading stops
> > working properly). The emphasis is put on the keyboard with
> > ATK_STATE_FOCUSED, so I decided to focus (pardon the pun) on the keyboard.
> 
> You mean with the patch the shift modifier stop working to select the items?
> 

No, the selection itself is fine. With Orca, if you click an item with the mouse, it’s read fine, but problems arise with modifier keys pressed. Ctrl-clicked items weren’t being focused at all (not as a consequence of a bug), but shift-clicked items are only read sometimes when not using atk_focus_tracker_notify(). The signals are emitted as they should be, so that had me quite confused.

My solution was to simply remove ATK focusing on mouse clicks.

This could be talked over with #a11y.
> > 
> > There’s nothing wrong with it per se.
> > 
> > I can consult with #a11y on this one.
> 
> I think the problem is me not understanding it more than the patch being not
> correct or so :)

Okay, disregard everything. My changes were based on a false premise, so I’ll redo it once more.

Feel free to not commit anything, haha.

Created attachment 326962 [details] [review]
canvas-item: use atk_object_notify_state_change()

Updated for the libnautilus-private merge.

Created attachment 326965 [details] [review]
canvas-container: rework canvas item focusing

So this one does the exact opposite: “fixes“ the mouse focusing.

Previously, only keyboard focus was being tracked. Now, keyboard /and/ mouse focus is being tracked. I added a flag, which indicates whether the focused icon was focused using the keyboard (so it either sets/unsets the “highlighted_as_keyboard_focus” property or the ATK_STATE_FOCUSED state).

I plastered the input handler functions with calls to set_focus(), because it’s somewhat cleaner than extending the selection functions.

/me removes newcomers keyword and hides

Thanks for taking a deeper look into it. Now I'm even more confused heh.
Let's talk about it on IRC, what do you think?

Created attachment 327012 [details] [review]
canvas-container: rework canvas item focusing

Review of attachment 327012 [details] [review]:

LGTM thanks!

Review of attachment 326962 [details] [review]:

sure

Review of attachment 326962 [details] [review]:

sure

Attachment 326674 [details] pushed as bd73a8e - canvas-item: use atk_component_get_extents()
Attachment 326678 [details] pushed as f3aabd3 - eel-canvas: don't override deprecated functions
Attachment 326962 [details] pushed as 3e498fc - canvas-item: use atk_object_notify_state_change()
Attachment 327012 [details] pushed as dea1979 - canvas-container: rework canvas item focusing