Bug 30997

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 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 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.

(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?

(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?

(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.

Created attachment 42317 [details] Layout Test - Take 2 Add the test in the Mac & Win Skipped list.

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!

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.

(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?

> 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 on attachment 42280 [details] proposed fix r=me.

Comment on attachment 42280 [details] proposed fix Clearing flags on attachment: 42280 Committed r50446: <http://trac.webkit.org/changeset/50446>

All reviewed patches have been landed. Closing bug.

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 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.

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 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 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).

Created attachment 43094 [details] Layout Test - Take 3.5 Second verse same as the first. Only git-master compatible. At the moment. :-) :-)

Comment on attachment 43094 [details] Layout Test - Take 3.5 Clearing flags on attachment: 43094 Committed r50921: <http://trac.webkit.org/changeset/50921>

All reviewed patches have been landed. Closing bug.