[webkit-reviews] review requested: [Bug 36128] [Gtk] nameFromChildren is obsolete : [Attachment 58479] new layout test which should work fine

Fri Jun 11 08:46:25 PDT 2010

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 36128: [Gtk] nameFromChildren is obsolete
https://bugs.webkit.org/show_bug.cgi?id=36128

Attachment 58479: new layout test which should work fine
https://bugs.webkit.org/attachment.cgi?id=58479&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(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).