[Webkit-unassigned] [Bug 121684] [ATK] Expose aria-describedby with ATK_RELATION_DESCRIBED_BY

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=121684


Mario Sanchez Prada <mario at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #221655|review?                     |review-
               Flag|                            |




--- Comment #7 from Mario Sanchez Prada <mario at webkit.org>  2014-01-20 08:24:12 PST ---
(From update of attachment 221655)
View in context: https://bugs.webkit.org/attachment.cgi?id=221655&action=review

I think the patch looks good already, but I think there are some things that would need some changes before landing.

> LayoutTests/ChangeLog:8
> +        Slightly modified test so that it could test aria-describedby with multiple id references.

What about "extending" the test VS modifying it? I mean, keeping the "one ID" only case and adding the new one with two IDs on top of it.

> LayoutTests/accessibility/aria-describedby-on-input-expected.txt:3
> -Allows you to specify the number of minutes after which the computer will self-destruct.
> +Allows you to specify the number of minutes after
> +which the computer will self-destruct.

Why is this line break showing up here? I would expect the two strings to be concatenated together with a " " in the middle.

I looked into the implementation in DRT/WKTR, but still is not obvious to me...

> Source/WebCore/accessibility/AccessibilityNodeObject.h:185
> +    virtual void elementsFromAttribute(Vector<Element*>& elements, const QualifiedName&) const override;

I think we normally write OVERRIDE (uppercase letters). Also, why do you need to make this method virtual if the implementation in the parent is going to be an empty one?

I see the point of being able to call it from a AccessibilityObject, but I'm not sure that alone justifies doing this, unless it made sense to move the whole method (implementation too) up to AccessibilityObject

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:248
> +                AccessibilityObject* axObject = coreObject->document()->axObjectCache()->getOrCreate(element);

The paranoid in me would add a null check for the document, but I don't think it's possible that you ever reach a null doc if you got this far (so I'm ok)

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:819
> +        if (atk_relation_get_relation_type(relation) == ATK_RELATION_DESCRIBED_BY) {

Nit. I would do if (atk_relation_get_relation_type(relation) != ATK_RELATION_DESCRIBED_BY) continue; here, to avoid nesting too much below

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:820
> +            GPtrArray* targetList = atk_relation_get_target(relation);

Not sure if this targetList can ever be NULL. Looking at AtkRelationSet code directly suggests that the internal pointer relation->target (returned by get_target()), could be NULL, although it's not clear whether that will ever happen in a real-life scenario.

In any case, I would add a null check here, maybe with an "early continue" as well ("if (!targetList) continue;")

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.