Created attachment 363918 [details]
Example GtkNotebook

run the example and observe the output inside Accercisser. The Image won't be accessible.

And here's the culprit:
static gint
gtk_notebook_page_accessible_get_n_children (AtkObject *accessible)
{
  return 1;
}

static AtkObject *
gtk_notebook_page_accessible_ref_child (AtkObject *accessible,
                                        gint       i)
{
  AtkObject *child_obj;
  GtkNotebookPageAccessible *page = NULL;

  if (i != 0)
    return NULL;

  page = GTK_NOTEBOOK_PAGE_ACCESSIBLE (accessible);
  if (!page->priv->child)
    return NULL;

  child_obj = gtk_widget_get_accessible (page->priv->child);
  g_object_ref (child_obj);

  return child_obj;
}

Created attachment 364056 [details] [review]
Patch to fix the issue

this should fix the issue mentioned in the bug.
This patch still needs review. 
The proposal is that GtkNotebookPage will always return 2 children instead of only 1. The first child will be the tab or the container that holds whatever is there while the second one will be actually the page contents.

Review of attachment 364056 [details] [review]:

In general lgtm. In any case, I think that it would be good to clarify somewhere, that the "tab label" can be more than a label (I have a suggestion below).. At first, looking at just the patch, I didn't understand why this was needed as the label text was exposed as the name of the page, until I read all the comments on the bug, and checked on code that the tab_label can be a widget with more than just a label (the name itself is slightly confusing).

Having said so, as mentioned on IRC, I'm not a gtk reviewer, so I can't really give the accepted status.

::: gtknotebookpageaccessible.c
@@ +129,1 @@
+  if ( i == 0 ) // first will be always the label or the component that contains the label 

I think that it would be more readable to use a switch here, even if it if just for three cases (child 0, child 1, plus wrong index).

Also the comment is somewhat confusing, as using label twice. I would prefer to not mention anything or something like:
"first child is the tab_label widget, that contains the real label, and in some cases additional widgets, like buttons, images, etc"

Review of attachment 364056 [details] [review]:

Thanks for your review, Alejandro.

Marcin, the style of the patch does not follow the GTK coding style: https://git.gnome.org/browse/gtk+/tree/docs/CODING-STYLE

Could you please fix that, in order to merge it in the Git repository?

Additionally, it would be stellar if you could attach a patch generated by `git format-patch` or, better yet, through `git bz`; see: https://wiki.gnome.org/Git/WorkingWithPatches

This would allow to keep track of the bug in the commit log, and to preserve authorship and attribution.

Thanks!

::: gtknotebookpageaccessible.c
@@ +116,3 @@
 gtk_notebook_page_accessible_get_n_children (AtkObject *accessible)
 {
+  return 2;

A comment here, saying:

/* A tab label widget can either be a GtkLabel or any container widget */

would be appreciated, as to avoid losing this information.

@@ +129,1 @@
+  if ( i == 0 ) // first will be always the label or the component that contains the label 

Coding style:

 - no additional spaces inside  the ()
 - comments should go inside the block, and should use the C /* … */ format

@@ +129,2 @@
+  if ( i == 0 ) // first will be always the label or the component that contains the label 
+  {

Coding style: need additional indentation level for the curly brackets, and for the block.

@@ +130,3 @@
+  {
+    label = gtk_notebook_get_tab_label (notebook, page->priv->child);
+    label_obj = gtk_widget_get_accessible(label);

Coding style: need additional space between function name and open parenthesis.

@@ +131,3 @@
+    label = gtk_notebook_get_tab_label (notebook, page->priv->child);
+    label_obj = gtk_widget_get_accessible(label);
+    g_object_ref(label_obj);

Coding style: need additional space between function name and open parenthesis.

@@ +132,3 @@
+    label_obj = gtk_widget_get_accessible(label);
+    g_object_ref(label_obj);
+    return label_obj;

You can merge these two lines as:

  return g_object_ref (label_obj);

@@ +134,3 @@
+    return label_obj;
+  }
+  else if (i == 1) { 

Coding style: curly brackets go on their own line, on a separate indentation level.

@@ +136,3 @@
+  else if (i == 1) { 
+    page = GTK_NOTEBOOK_PAGE_ACCESSIBLE (accessible);
+    if (!page->priv->child)

Coding style: please, use explicit NULL in pointer comparisons, i.e.

  if (page->priv->child == NULL)

@@ +143,3 @@
+    g_object_ref (child_obj);
+
+    return child_obj;

As above, you can do `return g_object_ref (child_obj);` instead of ref and return as separate statements.

@@ +145,3 @@
+    return child_obj;
+  } 
+  else {

Coding style: no need to use curly brackets for single statement blocks.

You also don't really need an `else` clause, here; just return NULL as the default value, to catch eventual missing conditions.

Created attachment 364109 [details] [review]
Patch to fix the issue

fix reported issues

Review of attachment 364109 [details] [review]:

Thanks for the quick update; there's one minor style issue still left, but with that fixed it looks good to me, too.

::: gtk/a11y/gtknotebookpageaccessible.c
@@ +128,3 @@
 
+  if (i == 0)
+  {

Coding style: curly braces go on a new indentation level, and the block they delimit as well. For instance:

  if (i == 0)
    {
      /* ... */
      label = ...
    }
  else if (i == 2)
    {
      page = ...
    }

@@ +135,3 @@
+  }
+  else if (i == 1)
+  {

Coding style: same as above.

Created attachment 364118 [details] [review]
Patch to fix the issue

had to make few more fixes

Review of attachment 364118 [details] [review]:

You seem to have added more checks, but those broke the coding style.

::: gtk/a11y/gtknotebookpageaccessible.c
@@ +131,2 @@
+  notebook = GTK_NOTEBOOK (gtk_accessible_get_widget (page->priv->notebook));
+  if (!notebook)

Coding style: pointer comparisons with NULL must be explicit.

@@ +133,3 @@
     return NULL;
 
+  if (i == 0 && gtk_notebook_get_show_tabs(notebook))

This is a bit weird, and I don't like the addition here.

If you want to handle the show-tabs option, please: split the patch, and create a new commit on top of the previous one.

The coding style is also wrong, as you're missing a space between the function name and the open parenthesis.

@@ +152,1 @@
+  return NULL;

So, now, if show-tabs is FALSE, you always return NULL, unless you ask for the second child, which looks wrong to me.

Again: this should be a separate patch, and it should be a check done before the whole index check:

  if (!gtk_notebook_get_show_tabs (notebook))
    return NULL;

Created attachment 364275 [details] [review]
Patch to fix the issue

Fixed reported issues and remove the gtk_notebook_get_show_tabs as this should be handled other way around

Created attachment 364484 [details] [review]
Return tab label as a notebook child

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/976.