Bug 117834 – [ATK] Adds support for aria-haspopup property.

• Component: Accessibility
• Status: RESOLVED FIXED

[Krzysztof Czech]

aria-haspopup indicates that the element has a popup context menu or sub-level menu.

[Krzysztof Czech]

Created attachment 205887 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 205887 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205887&action=review

> Tools/ChangeLog:13
> +        * WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:
> +        (WTR::attributeSetValueById):
> +        (WTR::AccessibilityUIElement::stringAttributeValue):
> +        (WTR::AccessibilityUIElement::hasPopup):

I wonder if it would be possible to implement the same functionality in DRT's AccessibilityUIElementAtk.cpp, so we can pass the test as well for WebKit1

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1098
> +    if (attributeValue.isNull())
> +        return false;
> +
> +    return attributeValue == "true" ? true : false;

I would probably write this way:

  if (attributeValue.isNull() || attributeValue == "false")
    return false;

  return true;

... but I'm fine with how it is now anyway. Your call.

[Krzysztof Czech]

(In reply to comment #2)
> (From update of attachment 205887 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205887&action=review
> 
> > Tools/ChangeLog:13
> > +        * WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:
> > +        (WTR::attributeSetValueById):
> > +        (WTR::AccessibilityUIElement::stringAttributeValue):
> > +        (WTR::AccessibilityUIElement::hasPopup):
> 
> I wonder if it would be possible to implement the same functionality in DRT's AccessibilityUIElementAtk.cpp, so we can pass the test as well for WebKit1

Ohh I forgot, you are right, I'll do it.
> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1098
> > +    if (attributeValue.isNull())
> > +        return false;
> > +
> > +    return attributeValue == "true" ? true : false;
> 
> I would probably write this way:
> 
>   if (attributeValue.isNull() || attributeValue == "false")
>     return false;
> 
>   return true;
> 
> ... but I'm fine with how it is now anyway. Your call.
Nice hint, sounds good to me as well, I'll change it.

[Krzysztof Czech]

Created attachment 205976 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


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

[Krzysztof Czech]

Created attachment 205978 [details]
Patch

[Krzysztof Czech]

> I wonder if it would be possible to implement the same functionality in DRT's AccessibilityUIElementAtk.cpp, so we can pass the test as well for WebKit1

WK1 support, done.
> 

> I would probably write this way:
> 
>   if (attributeValue.isNull() || attributeValue == "false")
>     return false;
> 
>   return true;
> 
> ... but I'm fine with how it is now anyway. Your call.
How about just return return attributeValue == "true" ? true : false;
I guess operator overloading for String will take care in case of nulls

[Mario Sanchez Prada]

(In reply to comment #7)
> > I wonder if it would be possible to implement the same functionality in DRT's AccessibilityUIElementAtk.cpp, so we can pass the test as well for WebKit1
> 
> WK1 support, done.

Cool, thanks

> > I would probably write this way:
> > 
> >   if (attributeValue.isNull() || attributeValue == "false")
> >     return false;
> > 
> >   return true;
> > 
> > ... but I'm fine with how it is now anyway. Your call.
> How about just return return attributeValue == "true" ? true : false;
> I guess operator overloading for String will take care in case of nulls

As I said, I'm fine with that too. Just sharing how I would probably do it, not necessarily meaning it's the best way to do it :).

In any case, if you're not checking nulls explicitly anyway then you could reduce it even more to a single statement:

  return attributeSetValueById(atk_object_get_attributes(ATK_OBJECT(m_element.get())), "aria-haspopup") == "true";

[Krzysztof Czech]

Created attachment 205982 [details]
Patch

[Krzysztof Czech]

> In any case, if you're not checking nulls explicitly anyway then you could reduce it even more to a single statement:
> 
>   return attributeSetValueById(atk_object_get_attributes(ATK_OBJECT(m_element.get())), "aria-haspopup") == "true";

This one look very nice, thanks.

[Krzysztof Czech]

Hello Chris, I already have an informal review of this patch. Could look at this, thanks.

[chris fleizach]

Comment on attachment 205982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205982&action=review

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:41
> +static void attributesClear(AtkAttributeSet* attributesSet)

this method should probably be named "clearAttributes()"

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:319
> +static String attributeSetValueById(AtkAttributeSet* attributeSet, const String& id)

this method seems like it should be named attributeValueForId after reading through what it does

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:326
> +            attributeValue.set(g_strdup(attribute->value));

Do you need to copy this to a GOwnPtr, or can you just return directly from here

with return String(g_strdup(attribute->value))

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1038
> +    return attributeSetValueById(atk_object_get_attributes(ATK_OBJECT(m_element)), "aria-haspopup") == "true";

you probably want to do an equalIgnoringCase in case someone does aria-haspop=True

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:83
> +static String attributeSetValueById(AtkAttributeSet* attributeSet, const String& id)

ditto

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1093
> +    return attributeSetValueById(atk_object_get_attributes(ATK_OBJECT(m_element.get())), "aria-haspopup") == "true";

ditto

[Krzysztof Czech]

Created attachment 206358 [details]
Patch

[Krzysztof Czech]

Rebased after r152397

[Krzysztof Czech]

> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:319
> > +static String attributeSetValueById(AtkAttributeSet* attributeSet, const String& id)
> 
> this method seems like it should be named attributeValueForId after reading through what it does

Yes, you are right, that sounds more accurate.

> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:326
> > +            attributeValue.set(g_strdup(attribute->value));
> 
> Do you need to copy this to a GOwnPtr, or can you just return directly from here
> 
> with return String(g_strdup(attribute->value))

I cannot directly return, I must clear AtkAttributeSet first.
 
> you probably want to do an equalIgnoringCase in case someone does aria-haspop=True

Yes, you are right

Thanks, applied all suggestions.

[Chris Dumez]

Comment on attachment 206358 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206358&action=review

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:59
> +    for (AtkAttributeSet* attributes = attributeSet; attributes; attributes = attributes->next) {

attributes -> attribute ?

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:62
> +            attributeValue.set(g_strdup(atkAttribute->value));

The strdup would not be needed if you freed the attributeSet list *after* constructing the return String. attributeValue would then be a const char*.

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:68
> +    return String(attributeValue.get());

String::fromUTF8(attributeValue)? iirc the string string is UTF-8 encoded, right?

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:64
> +            attributeValue.set(g_strdup(atkAttribute->value));

Same comments as above.

[Krzysztof Czech]

Created attachment 206517 [details]
Patch

[Krzysztof Czech]

> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:59
> > +    for (AtkAttributeSet* attributes = attributeSet; attributes; attributes = attributes->next) {
> 
> attributes -> attribute ?
I named it attributes because, it describes range of ATK attributes.
> 
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:62
> > +            attributeValue.set(g_strdup(atkAttribute->value));
> 
> The strdup would not be needed if you freed the attributeSet list *after* constructing the return String. attributeValue would then be a const char*.
> 
I agree, done.
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:68
> > +    return String(attributeValue.get());
> 
> String::fromUTF8(attributeValue)? iirc the string string is UTF-8 encoded, right?
> 
I guess you are right, corrected
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:64
> > +            attributeValue.set(g_strdup(atkAttribute->value));
> 
> Same comments as above.

[Krzysztof Czech]

Thanks for review.

[Build Bot]

Comment on attachment 206517 [details]
Patch

Attachment 206517 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1063080

New failing tests:
fullscreen/full-screen-iframe-with-max-width-height.html

[Build Bot]

Created attachment 206525 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Krzysztof Czech]

Created attachment 206526 [details]
Patch

[Chris Dumez]

Comment on attachment 206526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206526&action=review

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:54
> +static String atkAttributeSetValueForId(AtkObject* accessible, const String& id)

This could take a const char* in argument and return a CString. It does not look like we need a String at all (inside the function or the callers).

Also, I find the name a bit confusing because it sounds like it 'sets' something. How about getAttributeSetValueForId() ?

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56
> +static String atkAttributeSetValueForId(AtkObject* accessible, const String& id)

Same comments as for DRT.

[Krzysztof Czech]

Created attachment 206741 [details]
Patch

[Krzysztof Czech]

Created attachment 206742 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/accessibility/element-haspopup-expected.txt', u'LayoutTests/accessibility/element-haspopup.html', u'LayoutTests/accessibility/popup-button-title-expected.txt', u'LayoutTests/accessibility/popup-button-title.html', u'LayoutTests/platform/mac/accessibility/element-haspopup-expected.txt', u'LayoutTests/platform/mac/accessibility/element-haspopup.html', u'LayoutTests/platform/mac/accessibility/popup-button-title-expected.txt', u'LayoutTests/platform/mac/accessibility/popup-button-title.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp', u'Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp']" exit_code: 1
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:496:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 10 files


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

[Krzysztof Czech]

Created attachment 206744 [details]
Patch

[Krzysztof Czech]

> 
> This could take a const char* in argument and return a CString. It does not look like we need a String at all (inside the function or the callers).

There's one place when I need return String. It's an equalIgnoringCase function. It does not take two raw chars. I will have to convert one of those params to String.

> 
> Also, I find the name a bit confusing because it sounds like it 'sets' something. How about getAttributeSetValueForId() ?

Sounds good to me.
> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56
> > +static String atkAttributeSetValueForId(AtkObject* accessible, const String& id)
> 
Done

[Chris Dumez]

Comment on attachment 206744 [details]
Patch

Looks fine. r=me.

[WebKit Commit Bot]

Comment on attachment 206744 [details]
Patch

Clearing flags on attachment: 206744

Committed r152716: <http://trac.webkit.org/changeset/152716>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.