Bug 121684

<rdar://problem/15040496>

Created attachment 221468 [details] patch

Created attachment 221640 [details] patch

Attachment 221640 [details] did not pass style-queue: ERROR: Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:821: Missing space before ( in for( [whitespace/parens] [5] ERROR: Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:961: Missing space before ( in for( [whitespace/parens] [5] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 3 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 221642 [details] patch

Created attachment 221655 [details] patch

Comment on attachment 221655 [details] patch 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;")

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

(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

Created attachment 221979 [details] patch

Attachment 221979 [details] did not pass style-queue: ERROR: Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:824: Missing space before ( in for( [whitespace/parens] [5] ERROR: Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:981: Missing space before ( in for( [whitespace/parens] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 221982 [details] patch

> > > > 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 :) > I moved this method to AccessibilityObject and I think it does not have to be virtual either. It provides a common interface for both ANO and ARO and I doubt it could ever be customized by those classes.

Created attachment 221984 [details] patch

Comment on attachment 221984 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=221984&action=review > 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. I still don't quite understand why the line break shows up here. IMHO, the text from the two divs should be concatenated together and joined with a " ", not a line break. Chris, could you comment on this issue? Maybe it's me misunderstanding how this aria-describedby thing should work, or maybe it's another bug somewhere else > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:252 > + if (node && node->isHTMLElement()) { > + if (!toHTMLElement(node)->getAttribute(HTMLNames::aria_describedbyAttr).isEmpty()) { You could early return if it's empty, saving one nested block more. Or even combine the two nested ifs and early return, but that would perhaps be less readable.

> > 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. > > I still don't quite understand why the line break shows up here. IMHO, the text from the two divs should be concatenated together and joined with a " ", not a line break. > > Chris, could you comment on this issue? Maybe it's me misunderstanding how this aria-describedby thing should work, or maybe it's another bug somewhere else > Chris any comments regarding this issue ?

Comment on attachment 221984 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=221984&action=review >> LayoutTests/accessibility/aria-describedby-on-input-expected.txt:3 >> +which the computer will self-destruct. > > I still don't quite understand why the line break shows up here. IMHO, the text from the two divs should be concatenated together and joined with a " ", not a line break. > > Chris, could you comment on this issue? Maybe it's me misunderstanding how this aria-describedby thing should work, or maybe it's another bug somewhere else yea i'm also a little surprised that ariaDescribedByAttribute() would somehow get the newline It would indicate that static String accessibleNameForNode(Node* node) -> textUnderElement() returns a string in the first element that has a newline that surprises me. i don't think it's a show stopper and I don't think this patch is the cause of the problem, but it's something we should at least understand > Source/WebCore/accessibility/AccessibilityObject.cpp:2141 > + for (unsigned i = 0; i < size; ++i) { we should use for (auto name : idVector) > Source/WebCore/accessibility/AccessibilityObject.cpp:2144 > + if (idElement) this can be written if (Element* idElement = treeScope...) elements.append() > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:253 > + removeAtkRelationByType(relationSet, ATK_RELATION_DESCRIBED_BY); There's a ariaDescribedByAttribute() on the AXObject. We should make that public so you can call that directly, instead of calling the attribute directly on the Node object in the platform code > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:258 > + if (!axObject) this if can be combined with the line above

> >> LayoutTests/accessibility/aria-describedby-on-input-expected.txt:3 > >> +which the computer will self-destruct. > > > > I still don't quite understand why the line break shows up here. IMHO, the text from the two divs should be concatenated together and joined with a " ", not a line break. > > > > Chris, could you comment on this issue? Maybe it's me misunderstanding how this aria-describedby thing should work, or maybe it's another bug somewhere else > > yea i'm also a little surprised that ariaDescribedByAttribute() would somehow get the newline > It would indicate that > static String accessibleNameForNode(Node* node) -> textUnderElement() > returns a string in the first element that has a newline > that surprises me. i don't think it's a show stopper and I don't think this patch is the cause of the problem, but it's something we should at least understand Thanks this is something that I did not know. I thought that html is not obligated to take into account aria* attributes. I guess additional bug could be created to dig a bit more into this. > > > Source/WebCore/accessibility/AccessibilityObject.cpp:2141 > > + for (unsigned i = 0; i < size; ++i) { > > we should use > for (auto name : idVector) Sounds good. > > > Source/WebCore/accessibility/AccessibilityObject.cpp:2144 > > + if (idElement) > > this can be written > if (Element* idElement = treeScope...) > elements.append() Sounds good, thanks > > > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:253 > > + removeAtkRelationByType(relationSet, ATK_RELATION_DESCRIBED_BY); > > There's a ariaDescribedByAttribute() on the AXObject. We should make that public so you can call that directly, instead of calling the attribute directly on the Node object in the platform code How about to create something like ?: supportsAriaDescribedBy() { return !getAttribute(aria_describedbyAttr).isEmpty(); } I looked into the code and I see ariaDescribedByAttribute calls elementsFromAttribute. This method would be called twice in this context. > > > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:258 > > + if (!axObject) > > this if can be combined with the line above Sounds good, thanks.

(In reply to comment #18) > > >> LayoutTests/accessibility/aria-describedby-on-input-expected.txt:3 > > >> +which the computer will self-destruct. > > > > > > I still don't quite understand why the line break shows up here. IMHO, the text from the two divs should be concatenated together and joined with a " ", not a line break. > > > > > > Chris, could you comment on this issue? Maybe it's me misunderstanding how this aria-describedby thing should work, or maybe it's another bug somewhere else > > > > yea i'm also a little surprised that ariaDescribedByAttribute() would somehow get the newline > > It would indicate that > > static String accessibleNameForNode(Node* node) -> textUnderElement() > > returns a string in the first element that has a newline > > that surprises me. i don't think it's a show stopper and I don't think this patch is the cause of the problem, but it's something we should at least understand > Thanks this is something that I did not know. I thought that html is not obligated to take into account aria* attributes. I guess additional bug could be created to dig a bit more into this. > > > > > Source/WebCore/accessibility/AccessibilityObject.cpp:2141 > > > + for (unsigned i = 0; i < size; ++i) { > > > > we should use > > for (auto name : idVector) > Sounds good. > > > > > Source/WebCore/accessibility/AccessibilityObject.cpp:2144 > > > + if (idElement) > > > > this can be written > > if (Element* idElement = treeScope...) > > elements.append() > Sounds good, thanks > > > > > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:253 > > > + removeAtkRelationByType(relationSet, ATK_RELATION_DESCRIBED_BY); > > > > There's a ariaDescribedByAttribute() on the AXObject. We should make that public so you can call that directly, instead of calling the attribute directly on the Node object in the platform code > > How about to create something like ?: > supportsAriaDescribedBy() { return !getAttribute(aria_describedbyAttr).isEmpty(); } > Sounds good > I looked into the code and I see ariaDescribedByAttribute calls elementsFromAttribute. This method would be called twice in this context. > > > > > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:258 > > > + if (!axObject) > > > > this if can be combined with the line above > Sounds good, thanks.

Created attachment 222564 [details] patch

Attachment 222564 [details] did not pass style-queue: ERROR: Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1043: Missing space around : in range-based for statement [whitespace/colon] [4] ERROR: Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1048: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #20) > Created an attachment (id=222564) [details] > patch (In reply to comment #20) > Created an attachment (id=222564) [details] > patch Applied all Chris suggestions. Added also two additional methods to better deal with aria-describedby attribute: - supportsARIADescribedBy() - ariaDescribedByElements(...) - return elements that support aria-describedby

Created attachment 222565 [details] patch

Comment on attachment 222565 [details] patch lgtm

Comment on attachment 222565 [details] patch Clearing flags on attachment: 222565 Committed r163014: <http://trac.webkit.org/changeset/163014>

All reviewed patches have been landed. Closing bug.