Bug 30997 - [Gtk] Implement AtkDocument's attribute support
Summary: [Gtk] Implement AtkDocument's attribute support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 25531
  Show dependency treegraph
 
Reported: 2009-11-01 13:39 PST by Joanmarie Diggs
Modified: 2009-11-12 18:00 PST (History)
4 users (show)

See Also:


Attachments
proposed fix (3.80 KB, patch)
2009-11-01 13:39 PST, Joanmarie Diggs
no flags Details | Formatted Diff | Diff
Layout Test - Take 1 (8.83 KB, patch)
2009-11-01 22:11 PST, Joanmarie Diggs
jmalonzo: review-
Details | Formatted Diff | Diff
Layout Test - Take 2 (8.07 KB, patch)
2009-11-02 07:22 PST, Joanmarie Diggs
jmalonzo: review-
Details | Formatted Diff | Diff
Layout Test - Take 3 (10.98 KB, patch)
2009-11-12 09:35 PST, Joanmarie Diggs
gustavo: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Layout Test - Take 3.5 (10.89 KB, patch)
2009-11-12 13:32 PST, Joanmarie Diggs
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs 2009-11-01 13:39:39 PST
Created attachment 42280 [details]
proposed fix

+++ This bug was initially created as a clone of Bug #30964 +++

We need to implement the AtkDocument interface.
http://library.gnome.org/devel/atk/unstable/AtkDocument.html
Comment 1 Joanmarie Diggs 2009-11-01 22:11:35 PST
Created attachment 42301 [details]
Layout Test - Take 1

Given that this is my very first attempt at writing a Layout Test, I'm sure it's not right. :-) But it's something concrete from which you can direct/enlighten me.... I also have a few questions:

1. In bug 30964 comment #8, Jan says:

> Layout tests. We're not testing ATK APIs but testing the accessible
> characteristics of AX objects. AccessibilityController exposes some of this
> attributes that we can test in JavaScript. It's still missing a few
> implementations for Gtk though, but shouldn't be difficult to implement those.

This to me sounds like I should be doing something with AccessibilityController. :-) However, I don't grok why. Aren't these just more attributes of an HTML element? Granted it's a very specific HTML element which is likely (albeit not necessarily) the root document.... Therefore, for now, I added the support to AccessibilityUIElement and just did an Atk role test.

2. Is there some magic foo that occurs which causes these tests to pass on other platforms (in particular, win and mac)?

3. I assume that the way to test that the right thing is being exposed is to do Atk calls. Right?

4. The patch which implements the attribute functionality includes docType. In the case of local files, no doctype is returned. I haven't taken the time to ascertain if that is intended functionality or a bug. Regardless, what's the best way to add a layout test for doctype? (since I assume local files are preferred/expected).

Please be kind in your review. ;-) ;-)
Comment 2 Jan Alonzo 2009-11-02 02:37:08 PST
Comment on attachment 42280 [details]
proposed fix

> +    String value = String();
> +    if (!g_ascii_strcasecmp(attribute, "DocType") && coreDocument->doctype())
> +        value = coreDocument->doctype()->name();
> +    else if (!g_ascii_strcasecmp(attribute, "Encoding"))
> +        value = coreDocument->charset();
> +    else if (!g_ascii_strcasecmp(attribute, "URI"))
> +        value = coreDocument->documentURI();
> +    if (!value.isEmpty())
> +        return returnString(value);

Should this return an empty string if it's an empty value?
Comment 3 Jan Alonzo 2009-11-02 03:09:29 PST
Comment on attachment 42301 [details]
Layout Test - Take 1

Thanks for writing the test.

> diff --git a/LayoutTests/accessibility/document-attributes-expected.txt b/LayoutTests/accessibility/document-attributes-expected.txt
> new file mode 100644
> index 0000000..863339f
> --- /dev/null
> +++ b/LayoutTests/accessibility/document-attributes-expected.txt
> @@ -0,0 +1 @@
> +Passed

Is the result platform-specific? If so, we should put the -expected.txt into the platform-specific folders. See "add-platform-exceptions" option of run-webkit-tests to generate a platform-specific expected result.

> --- a/WebKitTools/DumpRenderTree/mac/AccessibilityUIElementMac.mm
> +++ b/WebKitTools/DumpRenderTree/mac/AccessibilityUIElementMac.mm
> @@ -577,3 +577,13 @@ JSStringRef AccessibilityUIElement::accessibilityValue() const
>      // FIXME: implement
>      return JSStringCreateWithCharacters(0, 0);
>  }
> +
> +JSStringRef AccessibilityUIElement::documentEncoding()
> +{
> +    return JSStringCreateWithCharacters(0, 0);
> +}
> +
> +JSStringRef AccessibilityUIElement::documentURI()
> +{
> +    return JSStringCreateWithCharacters(0, 0);
> +}
> diff --git a/WebKitTools/DumpRenderTree/win/AccessibilityUIElementWin.cpp b/WebKitTools/DumpRenderTree/win/AccessibilityUIElementWin.cpp
> index 2a8eaf6..89e9a5c 100644
> --- a/WebKitTools/DumpRenderTree/win/AccessibilityUIElementWin.cpp
> +++ b/WebKitTools/DumpRenderTree/win/AccessibilityUIElementWin.cpp
> @@ -392,3 +392,14 @@ JSStringRef AccessibilityUIElement::accessibilityValue() const
>  
>      return JSStringCreateWithCharacters(value.data(), value.length());
>  }
> +
> +
> +JSStringRef AccessibilityUIElement::documentEncoding()
> +{
> +    return JSStringCreateWithCharacters(0, 0);
> +}
> +
> +JSStringRef AccessibilityUIElement::documentURI()
> +{
> +    return JSStringCreateWithCharacters(0, 0);
> +}

Since Mac & Win are stubbed, I think this test will need to be added to Mac & Win's Skipped list (see LayoutTests/platform/{mac,win}/Skipped). Ditto with Qt.

You also missed the impl for Qt :).

r- for now until the above points are addressed.
Comment 4 Joanmarie Diggs 2009-11-02 04:14:59 PST
(In reply to comment #3)

> Is the result platform-specific? 

In the sense that Gtk is currently the only platform that seems to have a need to implement this, yes. However, the document type, url, and encoding are not a11y things. Nor would I expect them them to vary based on platform should the other platforms implement them down the road. So, in my mind, if there is magical test-skipping/ignoring foo (which there is I see from your later comment), the result would *not* be platform-specific.

All of that said, I'm a layout test n00b AND a WebKit n00b. :-) So given my response to the above, what are your thoughts? I'm happy to make it platform-specific, "just in case."
 
> Since Mac & Win are stubbed, I think this test will need to be added to Mac &
> Win's Skipped list (see LayoutTests/platform/{mac,win}/Skipped). Ditto with Qt.
> 
> You also missed the impl for Qt :).

I wouldn't say "missed." It's more like "drew a conclusion and intentionally failed to provide." :-) I looked in WebKitTools/DumpRenderTree/<platform>. Win, Mac, and Gtk all have AccessibilityController and AccessibilityUIElement implementations; Qt does not seem to have these things. My conclusion/assumption was that if Qt should have stubs for the document attributes, it would already have stubs (if not actual functions) for all of the other things being tested.

Should I add new AccessibilityController and AccessibilityUIElement implementations for Qt?
Comment 5 Joanmarie Diggs 2009-11-02 04:45:26 PST
(In reply to comment #2)
> (From update of attachment 42280 [details])
> > +    String value = String();
> > +    if (!g_ascii_strcasecmp(attribute, "DocType") && coreDocument->doctype())
> > +        value = coreDocument->doctype()->name();
> > +    else if (!g_ascii_strcasecmp(attribute, "Encoding"))
> > +        value = coreDocument->charset();
> > +    else if (!g_ascii_strcasecmp(attribute, "URI"))
> > +        value = coreDocument->documentURI();
> > +    if (!value.isEmpty())
> > +        return returnString(value);
> 
> Should this return an empty string if it's an empty value?

Personally, I don't think so. If an empty string is returned, it will wind up being added to the AttributeSet for the document (unless, of course, I prevent that from occurring in webkit_accessible_document_get_attributes).

While there is no official rule (that I've found) that states that one should not have an AtkAttribute with no value in an AtkAttributeSet, I've never seen a AtkAttribute without a value in an AtkAttributeSet.

Is there a reason why it should return an empty string rather than NULL which I'm failing to take into account?
Comment 6 Jan Alonzo 2009-11-02 05:16:49 PST
(In reply to comment #4)
> (In reply to comment #3)
> 
> > Is the result platform-specific? 
> 
> In the sense that Gtk is currently the only platform that seems to have a need
> to implement this, yes. However, the document type, url, and encoding are not
> a11y things. Nor would I expect them them to vary based on platform should the
> other platforms implement them down the road. So, in my mind, if there is
> magical test-skipping/ignoring foo (which there is I see from your later
> comment), the result would *not* be platform-specific.
> 
> All of that said, I'm a layout test n00b AND a WebKit n00b. :-) So given my
> response to the above, what are your thoughts? I'm happy to make it
> platform-specific, "just in case."

Let's keep them where it is now and add the test in the Mac & Win Skipped list.

> Should I add new AccessibilityController and AccessibilityUIElement
> implementations for Qt?

Nope. Don't worry. It seems that Qt's not building the Accessibility components of DRT so it's OK if it doesn't have it.
Comment 7 Joanmarie Diggs 2009-11-02 07:22:48 PST
Created attachment 42317 [details]
Layout Test - Take 2

Add the test in the Mac & Win Skipped list.
Comment 8 Joanmarie Diggs 2009-11-02 07:51:15 PST
Now I'm looking at the Unit Tests, in particular testatk.c, because in bug 30964 comment #7, Xan says:

> > I think we should start writing layout tests for these. Would that be possible
> > today or there's a plan to write layout tests for these changes soon?
> 
> Mmm, do you mean unit tests here? Or how could we test ATK APIs from layout
> tests in JavaScript?

And in bug 30964 comment #9, he says:

> OK. Since most of what we are doing here is implementing the ATK APIs I assumed
> you were referring to that. In any case, testing that all those APIs work as
> expected is, at least, as important as doing layout tests for any core AX
> functionality we are using or adding.

I aim to please and to prevent regressions. :-) Questions:

1. (Carried over from previous questions) The only way I could think to test the new functionality in a layout test was to make an Atk call. So that's what I did. Is that what I should have done?

2. If it is what I should have done, I believe that tests the relevant Atk APIs. In which case, what additional/different tests should go in a unit test for the new functionality?

3. testatk.c seems to me to be a great place to ... oh, I dunno.... test atk. :-) :-) :-) Given that the new functionality is (at least at the moment) Atk specific, the layout test I created needed stub functions for Windows and Mac. And it needed new entries for those two platforms to skip this new, irrelevant-to-them test. Would it be better to not do a layout test in this particular case and instead update testatk.c?

Please advise. Thanks!
Comment 9 chris fleizach 2009-11-02 08:30:02 PST
Joanmarie, 

It sounds like you are doing things correctly. You need to test the GTK wrapper, so it sounds correct that you should Skip the test for Win/mac and also stub out the DRT methods for those platforms

However, I think it would be nice if you could put the document attribute getting stuff into AccessibilityObject, so that if Mac/Win needed to implement similar things in the future, they wouldn't have to duplicate the functionality

that way, your wrapper just needs to call one method on the AX object to get the right info.
Comment 10 Joanmarie Diggs 2009-11-02 10:00:23 PST
(In reply to comment #9)
> Joanmarie, 

Friends, colleagues, and a11y folks call me Joanie. :-)

> It sounds like you are doing things correctly. You need to test the GTK
> wrapper, so it sounds correct that you should Skip the test for Win/mac and
> also stub out the DRT methods for those platforms

Yup, these things I understand. What I'm hung up on, and I readily admit that :-), is the fundamental purpose behind the layout tests versus that of the unit tests, and why what I want here is a layout test and not a unit test.

> However, I think it would be nice if you could put the document attribute
> getting stuff into AccessibilityObject, so that if Mac/Win needed to implement
> similar things in the future, they wouldn't have to duplicate the functionality

I'd be happy to do so.

> that way, your wrapper just needs to call one method on the AX object to get
> the right info.

Well.... If it goes into the cross-platform code, I'm tempted to make it three methods (1 for Encoding, 1 for URI, and 1 for DocType). Who's to say what constitutes *the* document attributes? Or do you feel it really should be one?
Comment 11 chris fleizach 2009-11-02 10:14:27 PST
> Yup, these things I understand. What I'm hung up on, and I readily admit that
> :-), is the fundamental purpose behind the layout tests versus that of the unit
> tests, and why what I want here is a layout test and not a unit test.
> 

I have never added any unit tests for accessibility, so I would recommend going with a layout test.

> Well.... If it goes into the cross-platform code, I'm tempted to make it three
> methods (1 for Encoding, 1 for URI, and 1 for DocType). Who's to say what
> constitutes *the* document attributes? Or do you feel it really should be one?

I think it's also acceptable to have three separate methods
Comment 12 Jan Alonzo 2009-11-03 01:52:16 PST
Comment on attachment 42280 [details]
proposed fix

r=me.
Comment 13 WebKit Commit Bot 2009-11-03 02:00:29 PST
Comment on attachment 42280 [details]
proposed fix

Clearing flags on attachment: 42280

Committed r50446: <http://trac.webkit.org/changeset/50446>
Comment 14 WebKit Commit Bot 2009-11-03 02:00:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Joanmarie Diggs 2009-11-11 19:08:55 PST
Reopening.

I might be really, really tired. Correction: I *know* I'm really, really tired. :-) But it does not look like the Layout Test (and changes to DRT) ever got committed.

Jan?
Comment 16 Jan Alonzo 2009-11-12 01:53:09 PST
Comment on attachment 42317 [details]
Layout Test - Take 2

>  LayoutTests/platform/mac/Skipped                   |    3 +++
>  LayoutTests/platform/win/Skipped                   |    1 +
>  WebKitTools/ChangeLog                              |   20 ++++++++++++++++++++
>  .../DumpRenderTree/AccessibilityUIElement.cpp      |   14 ++++++++++++++
>  .../DumpRenderTree/AccessibilityUIElement.h        |    2 ++
>  .../gtk/AccessibilityUIElementGtk.cpp              |   20 ++++++++++++++++++++
>  .../mac/AccessibilityUIElementMac.mm               |   10 ++++++++++
>  .../win/AccessibilityUIElementWin.cpp              |   11 +++++++++++
>  8 files changed, 81 insertions(+), 0 deletions(-)

The layout tests changes are missing a Changelog.

> +
> +    AtkDocument* axDocument = ATK_DOCUMENT(m_element);
> +    return JSStringCreateWithUTF8CString(atk_document_get_attribute_value(axDocument, "Encoding"));

No need for axDocument here. I think you can just pass it to atk_document_get_attribute_value directly.

> +}
> +
> +JSStringRef AccessibilityUIElement::documentURI()
> +{
> +    AtkRole role = atk_object_get_role(ATK_OBJECT(m_element));
> +    if (role != ATK_ROLE_DOCUMENT_FRAME)
> +        return JSStringCreateWithCharacters(0, 0);
> +
> +    AtkDocument* axDocument = ATK_DOCUMENT(m_element);
> +    return JSStringCreateWithUTF8CString(atk_document_get_attribute_value(axDocument, "URI"));

Ditto.

Patch looks good. But I'm going to say r- because of the missing Changelog for the layout test changes.
Comment 17 Joanmarie Diggs 2009-11-12 09:35:20 PST
Created attachment 43069 [details]
Layout Test - Take 3

Thanks for the review! Hopefully third time will be charm. :-)

> The layout tests changes are missing a Changelog.

Added.

> > +    AtkDocument* axDocument = ATK_DOCUMENT(m_element);
> > +    return JSStringCreateWithUTF8CString(atk_document_get_attribute_value(axDocument, "Encoding"));
> 
> No need for axDocument here. I think you can just pass it to
> atk_document_get_attribute_value directly.

Done.

> > +    AtkDocument* axDocument = ATK_DOCUMENT(m_element);
> > +    return JSStringCreateWithUTF8CString(atk_document_get_attribute_value(axDocument, "URI"));
> 
> Ditto.

Ditto. :-)
Comment 18 Gustavo Noronha (kov) 2009-11-12 11:04:08 PST
Comment on attachment 43069 [details]
Layout Test - Take 3

I think it's better to add testing in the same patch that adds the functionality, usually, since it is important that the new functionality is tested as soon as possible (meaning when it is committed), so I would recommend doing it that way for future patches =). Regarding, unit tests are platform-specific, there is no unit testing structure for webkit as a whole, so when you want to test anything that is not platform-specific API, Layout Tests are a better option. Thanks for the patch!
Comment 19 WebKit Commit Bot 2009-11-12 11:07:00 PST
Comment on attachment 43069 [details]
Layout Test - Take 3

Rejecting patch 43069 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Gustavo Noronha Silva', '--force']" exit_code: 1
Last 500 characters of output:
bilityUIElement.cpp.rej
patching file WebKitTools/DumpRenderTree/AccessibilityUIElement.h
Hunk #1 succeeded at 112 with fuzz 1 (offset 3 lines).
patching file WebKitTools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp
Hunk #1 succeeded at 476 (offset 21 lines).
patching file WebKitTools/DumpRenderTree/mac/AccessibilityUIElementMac.mm
Hunk #1 succeeded at 639 (offset 35 lines).
patching file WebKitTools/DumpRenderTree/win/AccessibilityUIElementWin.cpp
Hunk #1 succeeded at 431 (offset 20 lines).
Comment 20 Joanmarie Diggs 2009-11-12 13:32:41 PST
Created attachment 43094 [details]
Layout Test - Take 3.5

Second verse same as the first. Only git-master compatible. At the moment. :-) :-)
Comment 21 WebKit Commit Bot 2009-11-12 18:00:01 PST
Comment on attachment 43094 [details]
Layout Test - Take 3.5

Clearing flags on attachment: 43094

Committed r50921: <http://trac.webkit.org/changeset/50921>
Comment 22 WebKit Commit Bot 2009-11-12 18:00:10 PST
All reviewed patches have been landed.  Closing bug.