[ATK] accessibility/title-ui-element-correctness.html fails

[Zan Dobersek]

The accessibility/title-ui-element-correctness.html test that was added in r131871 is failing on the GTK 64-bit release builder. The debug builder hasn't processed the commit yet.

http://trac.webkit.org/changeset/131871
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=accessibility%2Ftitle-ui-element-correctness.html

Here's the diff:
--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/accessibility/title-ui-element-correctness-expected.txt 
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/accessibility/title-ui-element-correctness-actual.txt 
@@ -5,13 +5,13 @@
 
 PASS axElement('control1').titleUIElement().isEqual(axElement('label1')) is true
 PASS axElement('control2').titleUIElement().isEqual(axElement('label2')) is true
-PASS hasTitleUIElement(axElement('control3')) is false
+FAIL hasTitleUIElement(axElement('control3')) should be false. Was true.
 PASS document.getElementById('label3').setAttribute('for', 'control3'); axElement('control3').titleUIElement().isEqual(axElement('label3')) is true
-PASS var label4Element = createLabelWithIdAndForAttribute('label4', 'control4'); hasTitleUIElement(axElement('control4')) is false
+FAIL var label4Element = createLabelWithIdAndForAttribute('label4', 'control4'); hasTitleUIElement(axElement('control4')) should be false. Was true.
 PASS document.getElementById('container').appendChild(label4Element); hasTitleUIElement(axElement('control4')) is true
 PASS axElement('control4').titleUIElement().isEqual(axElement('label4')) is true
-PASS label4Element.parentElement.removeChild(label4Element); hasTitleUIElement(axElement('control4')) is false
-PASS hasTitleUIElement(axElement('control5')) is false
+FAIL label4Element.parentElement.removeChild(label4Element); hasTitleUIElement(axElement('control4')) should be false. Was true.
+FAIL hasTitleUIElement(axElement('control5')) should be false. Was true.
 PASS reparentNodeIntoContainer(document.getElementById('control5'), document.getElementById('label5')); axElement('control5').titleUIElement() != null is true
 PASS axElement('control5').titleUIElement().isEqual(axElement('label5')) is true
 PASS axElement('control6').titleUIElement().isEqual(axElement('label6b')) is true

[Krzysztof Czech]

*** Bug 112019 has been marked as a duplicate of this bug. ***

[Michal Pakula vel Rutka]

Currently only one test case fails:

 PASS reparentNodeIntoContainer(document.getElementById('control5'), document.getElementById('label5')); axElement('control5').titleUIElement() != null is true
 PASS axElement('control5').titleUIElement().isEqual(axElement('label5')) is true
 PASS axElement('control6').titleUIElement().isEqual(axElement('label6b')) is true
-PASS newLabel6Element = createLabelWithIdAndForAttribute('label6a', 'control6'); document.body.insertBefore(newLabel6Element, document.body.firstChild); axElement('control6').titleUIElement().isEqual(axElement('label6a')) is true
+FAIL newLabel6Element = createLabelWithIdAndForAttribute('label6a', 'control6'); document.body.insertBefore(newLabel6Element, document.body.firstChild); axElement('control6').titleUIElement().isEqual(axElement('label6a')) should be true. Was false.
 PASS document.body.removeChild(newLabel6Element); axElement('control6').titleUIElement().isEqual(axElement('label6b')) is true
 PASS successfullyParsed is true

In this test case a new label for control6 element is added before the existing one, and it should be returned when calling titleUIElement.
When setting a new relation for an object which currently has one, atk_relation_set_add_relation_by_type adds a relation as  next element of relation's target list, while in DRT/WTR implementation we take into account only the first target.

[Michal Pakula vel Rutka]

Created attachment 216163 [details]
proposed patch

This patch fixes the bug by removing current relations before adding a new one.

[Mario Sanchez Prada]

Comment on attachment 216163 [details]
proposed patch

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

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:194
> +static void removeAtkRelationFromRelationSetByType(AtkRelationSet* relationSet, AtkRelationType relationType)

I think removeAtkRelationByType is enough for the name

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:199
> +        if (atk_relation_get_relation_type(relation) == ATK_RELATION_LABELLED_BY) {

You should be comparing against your relationType parameter, not that hardcoded case

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:201
> +            break;

You should not break here, as the same type of AtkRelation might exist between one base object and one or more target objects. So, if you are removing by type you should remove them all the relationships matching that type

[Michal Pakula vel Rutka]

Comment on attachment 216163 [details]
proposed patch

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

>> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:194
>> +static void removeAtkRelationFromRelationSetByType(AtkRelationSet* relationSet, AtkRelationType relationType)
> 
> I think removeAtkRelationByType is enough for the name

OK

>> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:199
>> +        if (atk_relation_get_relation_type(relation) == ATK_RELATION_LABELLED_BY) {
> 
> You should be comparing against your relationType parameter, not that hardcoded case

my bad, I will fix it

>> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:201
>> +            break;
> 
> You should not break here, as the same type of AtkRelation might exist between one base object and one or more target objects. So, if you are removing by type you should remove them all the relationships matching that type

I am not sure about it. In atk_relation_set_add_relation_by_type, there is a check if relation of that type is present in relation set. If yes, a new target is added, but no new relation is added to relation set - so I assume there should be only one relation of given type in relation set.

[Mario Sanchez Prada]

(In reply to comment #5)
> [...]
> > You should not break here, as the same type of AtkRelation might exist 
> > between one base object and one or more target objects. So, if you are
> > removing by type you should remove them all the relationships matching that 
> > type
> 
> I am not sure about it. In atk_relation_set_add_relation_by_type, there is a 
> check if relation of that type is present in relation set. If yes, a new
> target is added, but no new relation is added to relation set - so I assume
> there should be only one relation of given type in relation set.

You are right. I've checked the code in atkrelationset.c and it's quite clear about that. Thanks for noticing

[Michal Pakula vel Rutka]

Created attachment 216174 [details]
fixes after review

[WebKit Commit Bot]

Comment on attachment 216174 [details]
fixes after review

Clearing flags on attachment: 216174

Committed r158757: <http://trac.webkit.org/changeset/158757>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.