[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 #9 from Mario Sanchez Prada <mario at webkit.org>  2014-01-21 06:20:00 PST ---
(In reply to comment #8)
> [...]
> > > 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. 

Yes, but still I think the resultant text should have a space in the middle, not a line break.

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

Forget about this. It's 'override' from now on, not OVERRIDE (see webkit-dev)

> 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.
> 
Ok. I buy it :)

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

Ok

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