The recommendation we've had so far is that you should make such actions available in the context menu as well.

Except that there is no context menu anymore, and it seems a bit crazy to bring it back just because accessibility is unable to see that entry icon button...

Besides, the contextual/popup menu items were meant for actions that are specific to the currently selected items in a listview/iconview... and the icon I need to click in the search entry does not fit those criteria, it's an action that is related to search.

I guess GtkEntry could advertise those icons as children of its accessible somehow; I am not a great expert on ATK, but I can see two possible alternatives:
1) make GtkEntryAccessible implement get_n_children/ref_child and have a GtkEntryIconAccessible class for those icons
2) expose those icons as actions on the entry itself when they're sensitive - they could take the name of the relevant tooltip property. This sounds easier, but we would probably lose that information in case the icons are not sensitive or not activatable

I think 1) would be more robust overall if we want to expose those icons.

Created attachment 227737 [details] [review]
Proposed patch.

I've created a patch to add a GtkEntryIconAccessible class. I am unsure whether I am doing the right thing for gtk_entry_icon_accessible_get_extents and the like, so, if someone familiar with the GtkEntry code wanted to review that code in particular, then that would be helpful.

Review of attachment 227737 [details] [review]:

Thanks for the patch; I'm not an expert on ATK, so apologies in advance if some of these comments don't make sense.

::: gtk/a11y/gtkentryaccessible.c
@@ +154,3 @@
+      atk_state_set_add_state (set, ATK_STATE_DEFUNCT);
+    if (entry_set)
+static void

g_clear_object()

@@ +244,3 @@
+  event.button.send_event = TRUE;
+  event.button.time = GDK_CURRENT_TIME;
+    {

Synthesizing the event and emitting it like this is quite evil (I don't see many alternatives though)...does it work? Maybe we should also care to set the event coordinates to a point inside the icon area?

@@ +328,3 @@
+  atk_component_get_position (ATK_COMPONENT (icon->entry), x, y, coord_type);
+  if (*x == G_MININT)
+                       gtk_entry_icon_accessible_remove_entry,

I don't really understand what this code is trying to do. How can it happen that the entry doesn't return a valid position?

@@ +334,3 @@
+  gtk_entry_get_icon_area (gtk_entry, icon->pos, &icon_area);
+  *x += icon_area.x;
+}

This should be fine, as I think coordinates returned by gtk_entry_get_icon_area() are relative to the entry itself.

@@ +347,3 @@
+  GtkWidget *widget;
+
+  gtk_entry_icon_accessible_invalidate (icon);

I think this code here is fine.

@@ +514,3 @@
+      if (gtk_entry_get_icon_storage_type (gtk_entry, GTK_ENTRY_ICON_PRIMARY) != GTK_IMAGE_EMPTY && !priv->icons [0])
+        {
+          priv->icons [0] = gtk_entry_icon_accessible_new (entry, GTK_ENTRY_ICON_PRIMARY);

I think it'd be clearer to use GTK_ENTRY_ICON_PRIMARY/GTK_ENTRY_ICON_SECONDARY instead of 0/1 all around the function when accessing priv->icons[]. Also you don't need spaces before square brackets.

@@ +520,3 @@
+      else if (gtk_entry_get_icon_storage_type (gtk_entry, GTK_ENTRY_ICON_PRIMARY) == GTK_IMAGE_EMPTY && priv->icons [GTK_ENTRY_ICON_PRIMARY])
+        {
+          priv->icons [0] = gtk_entry_icon_accessible_new (entry, GTK_ENTRY_ICON_PRIMARY);

Why do you need to call gtk_entry_icon_accessible_invalidate() manually here and below? It will get called in finalize() by the icon accessible itself. Or does it need to run before children-changed is emitted?

@@ +524,3 @@
+                                 priv->icons [0], NULL);
+          g_object_unref (priv->icons [0]);
+                                 priv->icons [0], NULL);

g_clear_object()

@@ +542,3 @@
+                                 priv->icons [1], NULL);
+          g_object_unref (priv->icons [1]);
+        }

g_clear_object()

@@ +588,3 @@
+    }
+  else if (g_strcmp0 (pspec->name, "primary-icon-sensitive") == 0)
+        {

Shouldn't primary/secondary-icon-activatable be used for the ENABLED state instead (also in ref_state_set() above)

@@ +634,3 @@
+  widget = gtk_accessible_get_widget (GTK_ACCESSIBLE (obj));
+  if (widget == NULL)
+  GtkEntry *entry;

Not a huge fan of these checks, but it looks like other accessibles do the same.

Is it possible to restructure the code in the future so that get_n_children() and ref_child() are guaranteed to be called with a valid widget?

@@ +684,3 @@
+  if (!priv->icons [pos])
+    priv->icons [pos] = gtk_entry_icon_accessible_new (accessible, pos);
+

Should we make sure to unref these accessibles on finalize?

::: gtk/gtkentry.c
@@ +8810,3 @@
+
+  g_object_notify (G_OBJECT (entry), 
+                   icon_pos == GTK_ENTRY_ICON_PRIMARY ? "primary-icon-tooltip-text" : "secondary-icon-tooltip-text");

This looks like it should go in a separate patch.

(In reply to comment #5)


Thanks for the review.

> @@ +244,3 @@
> +  event.button.send_event = TRUE;
> +  event.button.time = GDK_CURRENT_TIME;
> +    {
> 
> Synthesizing the event and emitting it like this is quite evil (I don't see
> many alternatives though)...does it work? Maybe we should also care to set the
> event coordinates to a point inside the icon area?

I based that code on some old gtk 2 accessibility code. I haven't tested it; I should do that before sending another patch. Also, I'm not sure if implementing AtkAction is strictly necessary--it is possible to synthesize a mouse click via AT-SPI, for instance. GtkButtonAccessible has a similar action, although GtkButton's click signal doesn't take a GdkEvent.

> @@ +328,3 @@
> +  atk_component_get_position (ATK_COMPONENT (icon->entry), x, y, coord_type);
> +  if (*x == G_MININT)
> +                       gtk_entry_icon_accessible_remove_entry,
> 
> I don't really understand what this code is trying to do. How can it happen
> that the entry doesn't return a valid position?

I guess it will if the widget is off-screen; ie, gtk_widget_accessible_on_screen()

> @@ +520,3 @@
> +      else if (gtk_entry_get_icon_storage_type (gtk_entry,
> GTK_ENTRY_ICON_PRIMARY) == GTK_IMAGE_EMPTY && priv->icons
> [GTK_ENTRY_ICON_PRIMARY])
> +        {
> +          priv->icons [0] = gtk_entry_icon_accessible_new (entry,
> GTK_ENTRY_ICON_PRIMARY);
> 
> Why do you need to call gtk_entry_icon_accessible_invalidate() manually here
> and below? It will get called in finalize() by the icon accessible itself. Or
> does it need to run before children-changed is emitted?

This way it will send a state-chane:defunct notification here, even if something else (ie, at-spi2-atk) is holding a reference to it, although at-spi2-atk should not be holding a hard reference to it in this case, so I think it is more defensive than strictly necessary.

> @@ +588,3 @@
> +    }
> +  else if (g_strcmp0 (pspec->name, "primary-icon-sensitive") == 0)
> +        {
> 
> Shouldn't primary/secondary-icon-activatable be used for the ENABLED state
> instead (also in ref_state_set() above)

I'm not completely sure, but I think you're right. I'll make the change.

> @@ +634,3 @@
> +  widget = gtk_accessible_get_widget (GTK_ACCESSIBLE (obj));
> +  if (widget == NULL)
> +  GtkEntry *entry;
> 
> Not a huge fan of these checks, but it looks like other accessibles do the
> same.
> 
> Is it possible to restructure the code in the future so that get_n_children()
> and ref_child() are guaranteed to be called with a valid widget?

In theory they shouldn't be needed anymore as long as a state-changed:defunct has been sent and atk calls to other objects don't return a reference to the now-defunct object, since at-spi2-atk removes the mapping from the D-Bus path to the atk object when the object is marked defunct. If we're going to stop using these kinds of checks, then I think that we should remove them systematically and see if anything crashes, since I'm not sure if/how much we've tested without them. Benjamin might have removed some similar checks a while ago.

> @@ +684,3 @@
> +  if (!priv->icons [pos])
> +    priv->icons [pos] = gtk_entry_icon_accessible_new (accessible, pos);
> +
> 
> Should we make sure to unref these accessibles on finalize?

Yes. Thanks for the catch.

Created attachment 228703 [details] [review]
Revised patch.

Use enum when accessing icon array.
g_object_unref + assign -> g_clear_object in some cases.
Use activatable, rather than sensitive, for ATK_STATE_ENABLED.
Set x and y when simulating a click. (I've tested that the code does, in fact, send the event.)

Created attachment 228704 [details] [review]
Patch to send a notification when a GtkEntry icon's tooltip text changes.

Review of attachment 228704 [details] [review]:

Looks good.

Review of attachment 228703 [details] [review]:

Thanks for the update, this looks good to me now.

Attachment 228704 [details] committed as 77c0f9..
Attachment 228703 [details] committed as b77434.