Bug 36128 – [Gtk] nameFromChildren is obsolete

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

Given an HTML element which contained text, such as:

  <label for="foo">My <u>Very</u> Nifty<br />Label</label>
  
WebKitGtk *used* to expose the following accessible hierarchy to ATs:

  -> ATK_ROLE_LABEL (no name, no text)
     -> ATK_ROLE_TEXT (name, text == My)
     -> ATK_ROLE_TEXT (name, text == Very)
     -> ATK_ROLE_TEXT (name, text == Nifty)
     -> ATK_ROLE_TEXT (name, text == Label)

As a result, it was necessary to piece together an accessible name for certain objects. nameFromChildren did this.

We have since flattened/collapsed the accessible hierarchy w.r.t. text objects. In addition, objects do not automatically derive their names from their accessible text. So the above is now exposed to ATs as:

  -> ATK_ROLE_LABEL (no name, text = My Very Nifty Label)
  
These changes make WebKitGtk+ much more consistent with how other Gtk+ apps behave. They also render nameFromChildren obsolete. It should be removed; callers should instead look to the AtkText interface implemented by the objects in question.

[Joanmarie Diggs]

Created attachment 50725 [details]
proposed fix - no test (yet)

This patch fixes the problem and causes labels (and cell headers) to once again correctly report their name/description.

I will attach the test separately. The reason being that something* is preventing accessible text from being exposed with layout tests (or something that I'm doing in my layout test).

*I'm still digging, but the difference seems to be in TextIterator.cpp: plainTextToMallocAllocatedBuffer works as expected during actual use (i.e. by a user of AT accessing content). With my layout test, it returns NULL. :-(

[WebKit Review Bot]

Attachment 50725 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:146:  webkit_accessible_text_get_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joanmarie Diggs]

The reported style failure is not the result of my creating a new method with underscores; I merely added a declaration of an existing method so that other methods could call it.

[Joanmarie Diggs]

Created attachment 50727 [details]
new layout test which should work, but doesn't

This new layout test should work, but doesn't. Instead of 'Full Name:', the name of the entry is ''. This seems to be because plainTextToMallocAllocatedBuffer returns NULL, but only during the running of the layout test. If I examine the same content using Accerciser and GtkLauncher, the entry has the expected name.

I'll keep looking, but if anyone has suggestions/ideas, I'd love to hear them. Thanks!

[Joanmarie Diggs]

Here's what appears to be going on with my layout test:

webkit_accessible_text_get_text tries to get the text for the label via AccessibilityRenderObject::textUnderElement().

AccessibilityRenderObject::textUnderElement() returns plainText(rangeOfContents(node).get()).  When using GtkLauncher, the String returned contains 'Full Name:' as expected; but when the very same content is run in my layout test, the returned String is empty.

plainText() is returning an empty String because its call to plainTextToMallocAllocatedBuffer() returns NULL because bufferLength == 0.

bufferLength is based on the length of the TextIterator. This length seems to incremented in emitText(). emitText() is not getting called in the layout test because TextIterator::handleTextNode() is returning at this point:

    if (!renderer->firstTextBox() && str.length() > 0) {
        m_lastTextNodeEndedWithCollapsedSpace = true; // entire block is collapsed space
        return true;
    }

So.... Given the same, simple HTML content, why would a RenderText object fail to have a firstTextBox when run as a layout test, but behave as expected otherwise?

[Eric Seidel (no email)]

Comment on attachment 50725 [details]
proposed fix - no test (yet)

Please re-attach with the functioning test and the change in one patch.

[Joanmarie Diggs]

(In reply to comment #6)
> (From update of attachment 50725 [details])
> Please re-attach with the functioning test and the change in one patch.

Understood. However:

1. The fix (patch 1) is needed and actually works in terms of fixing the bug so that assistive technologies can provide better access to WebKitGtk content.

2. I think there is some other change (i.e. a not-yet-figured-out patch 3) that is preventing the test (patch 2) from working. I have no idea what that change is. I could use some help in figuring that out and have asked both in this bug and in IRC.

So, what this boils down to seems to be this: My failure to understand some issue in DRT means that my fix (which I've verified to work and which users who are blind need) cannot be accepted. It would be awesome if someone could help with the DRT issue -- or if the fix could be accepted and a different bug opened for the test and DRT issue.

[Mario Sanchez Prada]

After some investigation I found that this patch would solve the issue, although I'm not sure that's the right way to go:

diff --git a/WebCore/accessibility/AccessibilityRenderObject.cpp b/WebCore/accessibility/AccessibilityRenderObject.cpp
index 071bf5d..15e52ee 100644
--- a/WebCore/accessibility/AccessibilityRenderObject.cpp
+++ b/WebCore/accessibility/AccessibilityRenderObject.cpp
@@ -1007,7 +1007,16 @@ String AccessibilityRenderObject::textUnderElement() const
             // catch stale WebCoreAXObject (see <rdar://problem/3960196>)
             if (frame->document() != node->document())
                 return String();
-            return plainText(rangeOfContents(node).get());
+            String ret = plainText(rangeOfContents(node).get());
+
+            // try the first (text) descendant as a last resort
+            if (!ret.length()) {
+                Node *descendant = node->firstDescendant();
+                if (descendant->isTextNode())
+                    ret = descendant->textContent(false);
+            }
+
+            return ret;
         }
     }

The rationale behind this is that I've observed that, when running the test through the DRT, the return value of plainText() is always an empty String, while directly accessing to the text content of its first descendant (in case it's a text node) returns the right value.

I know this is unlikely to be the right solution, but sharing it anyway just in case someone else, which more knowledge than me at this moment, could comment something about it... or even in case it rang any bell that could lead to the right solution (in case it's not this one).

Btw, Accerciser keeps working fine with this bug applied, and I wasn't able to find any test failing because of this change.

[Mario Sanchez Prada]

Created attachment 58479 [details]
new layout test which should work fine

(In reply to comment #5)
> [...]
> bufferLength is based on the length of the TextIterator. This length seems to incremented in emitText(). emitText() is not getting called in the layout test because TextIterator::handleTextNode() is returning at this point:
> 
>     if (!renderer->firstTextBox() && str.length() > 0) {
>         m_lastTextNodeEndedWithCollapsedSpace = true; // entire block is collapsed space
>         return true;
>     }
> 
> So.... Given the same, simple HTML content, why would a RenderText object fail to have a firstTextBox when run as a layout test, but behave as expected otherwise?

After digging on this issue for some time (as well as trying to figure out how DRT and the whole a11y stack works in WebKit :-)), I think I finally found the reason while this test was failing with DRT but no with Accerciser: it's just that the layout() process has not been completed when you ask for the label's title in the test, so the text box for that piece of text is still not created, resulting in what we already know that is an empty string being returned :-/.

In other words, the failing flow would be:

  1. In the test you just ask for 'child.title' while the layout() still didn't happen
  2. webkit_accessible_text_get_name for the entry boils down to webkit_accessible_text_get_text for the corresponding label, which actually falls back to AccessibilityRenderObject::textUnderElement().
  3. AccessibilityRenderObject::textUnderElement() needs to call to plainText(), which internally ends up creating a new TextIterator for the range passed to plainText().
  4. However, the text iterator created when running DRT is an empty one, as the call to renderer->firstTexBox() returns NULL (since the layout() still didn't happen) and no valid text is finally returned by webkit_accessible_text_get_name().

So, what I did to fix this is to follow what I see in many other tests in LayoutTests (which also depend on the page being properly loaded and layed-out first) and change your proposed test not to do anything until the page has completely been loaded (<body onload="startTest();">...</body>) and, on top of that, to add a timeout to ensure that there's enough time after that to make sure the test has been renderized. 

Moreover, I've added another timeout to ensure the js-test-post.js file has been loaded at the end of the test before calling to notifyDone, so the expected result and the actual one match perfectly.

Perhaps there's a better way to do this (not so many timeouts) but at this point this is the best way I found to fix this (which in the other hand seems to be a common practice in many tests) and I couldn't resist to share this findings now :-), hopefully to get some feedback as well in how to improve it (for instance, if it were possible to get notified somehow in the test when the layou() process has been done, and then to call to startTest(), it would be great as we'd avoid the timeout).

Looking forward to hearing some opinions, for the time being here you have the patch replacing your second one (just a new test).

[Joanmarie Diggs]

Mario, as we discussed Friday in IRC, if you could combine the working test and the fix into a single patch and flag the result for review, perhaps we could get Xan or Eric to take a look.

We need this fix a) in general to make content accessible to users who are blind and b) for bug 30896. (After all, if we get the column header, but cannot get the contents from that cell, the column header is useless -- not to mention inaccessible).

Thanks!

[Mario Sanchez Prada]

(In reply to comment #10)
> Mario, as we discussed Friday in IRC, if you could combine the working test and the fix into a single patch and flag the result for review, perhaps we could get Xan or Eric to take a look.

That's the plan, thanks for all the support and help :-)
 
> We need this fix a) in general to make content accessible to users who are blind and b) for bug 30896. (After all, if we get the column header, but cannot get the contents from that cell, the column header is useless -- not to mention inaccessible).

My first priority for tomorrow morning is to work on this bug and try a suggestion coming from Martin Robinson that could help a lot to get a more general solution, let's see if it works (hope so!)

Once I'm done with this, next step would be bug 30869. Let'see if we can get rid of these two once and for all in a row! :-)

[Mario Sanchez Prada]

(In reply to comment #11)
> [...]
> Once I'm done with this, next step would be bug 30869. Let'see if we can get rid of these two once and for all in a row! :-)

I meant bug 30896, of course!

[Mario Sanchez Prada]

Created attachment 58770 [details]
combined patch (code+test) to fix this bug

(In reply to comment #11)
> [...]
> My first priority for tomorrow morning is to work on this bug and try a 
> suggestion coming from Martin Robinson that could help a lot to get a more 
> general solution, let's see if it works (hope so!)

I've tried mrobinson's suggestion[*] but couldn't get it working with changes in DRT only: I always needed anyway some timeouts to ensure layout() has been called when asking for the title of the <label> element.

Hence, I'm now attaching a combined patch with Joanmarie's patch + the new test, where I tried to reduce those timeouts to a minimum (while reasonable) value.

Let's see if we can get some review on this then and perhaps some advice to get, if possible, a more general solution (perhaps I was doing something wrong wrt mrobinson's suggestion).

> Once I'm done with this, next step would be bug 30869. Let'see if we can get 
> rid of these two once and for all in a row! :-)

I'll step into this one now, while waiting for review on this combined patch.

[*]  Try to call to webkit_web_frame_layout() before dumping the tree

[WebKit Review Bot]

Attachment 58770 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:146:  webkit_accessible_text_get_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mario Sanchez Prada]

Created attachment 58885 [details]
combined patch (code+test) to fix this bug

Attached a new version of the combined patch with a better test case, where I got rid of the second (and really weird) timeout, to ensure that notifyDone() was called after the post JS file being successfully loaded.

I found that just setting window.jsTestIsAsync to true is far more convenient for this, and also avoids all the mess about dynamically loading the post JS file to avoid calling to finishJSTest() to early, while keeping the same functionality, so I did it that way (and also reduced a bit the code in the test).

What I did not find yet is a way to get rid of the other timeout, which must be there (even when is set to 0ms) to kind of simulate a g_idle_add() in JS, and therefore ensure the layout has been properly done before checking the results. I know this is weird but (1) after deeply checking DRT I still haven't found a way to fix it there (if possible), and on top of that (2) I've seen more tests needing to do the same thing so I guess the current patch proposal could be good enough for now.

Hopefully in the future we will be able to find a more general solution in DRT code to avoid this problems (if possible), but so far whatever I've tried didn't work in a consistent way, and current proposal is the only one I got that didn't break any other tests.

Eager to read some opinions/review on this last patch :-)

[WebKit Review Bot]

Attachment 58885 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:146:  webkit_accessible_text_get_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mario Sanchez Prada]

(In reply to comment #16)
> Attachment 58885 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:146:  webkit_accessible_text_get_text is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> Total errors found: 1 in 5 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

As commented in a former attachment, this is something I couldn't avoid since the current style in that file is to name those kind of functions that way. Please let me know if there's a better way to work around this than just putting a comment in bugzilla as I'm doing right now

[chris fleizach]

Comment on attachment 58885 [details]
combined patch (code+test) to fix this bug


> @@ -1501,8 +1493,8 @@ static AtkObject* webkit_accessible_table_get_caption(AtkTable* table)
>  static const gchar* webkit_accessible_table_get_column_description(AtkTable* table, gint column)
>  {
>      AtkObject* columnHeader = atk_table_get_column_header(table, column);
> -    if (columnHeader)
> -        return returnString(nameFromChildren(core(columnHeader)));
> +    if (columnHeader && ATK_IS_TEXT(columnHeader))
> +        return webkit_accessible_text_get_text(ATK_TEXT(columnHeader), 0, -1);
>  

> +    if (rowHeader && ATK_IS_TEXT(rowHeader))
> +        return webkit_accessible_text_get_text(ATK_TEXT(rowHeader), 0, -1);


Can ATK_IS_TEXT check for nil, so you don't need to check for nil and ATK_IS_TEXT?

[chris fleizach]

r=me too

[Xan Lopez]

Comment on attachment 58885 [details]
combined patch (code+test) to fix this bug

Not checking for null-ness will give runtime errors, so I think we shouldn't do that. Setting cq+

[WebKit Commit Bot]

Comment on attachment 58885 [details]
combined patch (code+test) to fix this bug

Clearing flags on attachment: 58885

Committed r61730: <http://trac.webkit.org/changeset/61730>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.