[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





--- Comment #8 from Krzysztof Czech <k.czech at samsung.com>  2014-01-21 04:44:14 PST ---
> 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.

Sounds good.
> 
> > 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 think this line break is here because, I divided this text between two ids (description1 and description2) and put under two div's tags and html just reads inner text.

Concatenated string is the result of calling helpText. 
> 
> 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?

Regarding override instead of OVERRIDE, this is after r162139
I always thought that AccessibilityObject class is kind of an abstract class that provides an interface for AccessibilityNodeObject and for AccessibilityRenderObject so that virtual callings may happen. This is a reason this method has an empty body.

AccessibilityRenderObject derives the whole interface from AO and ANO and it will have this method out of the box.

Other thing is that ANO is something between AO and ARO and having body of this method there might look more natural.
> 
> 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

I realized this might be a good idea of moving its body directly 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;")

I will change this method a bit in the next iteration.

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