RESOLVED FIXED 99825
[ATK] accessibility/title-ui-element-correctness.html fails 
https://bugs.webkit.org/show_bug.cgi?id=99825
Summary [ATK] accessibility/title-ui-element-correctness.html fails
Zan Dobersek
Reported 2012-10-19 03:00:43 PDT
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
Attachments
proposed patch (6.13 KB, patch)
2013-11-06 04:11 PST, Michal Pakula vel Rutka 		mario: review-
DetailsFormatted DiffDiff
fixes after review (6.10 KB, patch)
2013-11-06 07:37 PST, Michal Pakula vel Rutka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Krzysztof Czech
Comment 1 2013-10-09 01:55:45 PDT
*** Bug 112019 has been marked as a duplicate of this bug. ***
Michal Pakula vel Rutka
Comment 2 2013-11-06 03:23:59 PST
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
Comment 3 2013-11-06 04:11:55 PST
Created attachment 216163 [details] proposed patch This patch fixes the bug by removing current relations before adding a new one.
Mario Sanchez Prada
Comment 4 2013-11-06 05:17:30 PST
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 5 2013-11-06 05:45:56 PST
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
Comment 6 2013-11-06 07:26:15 PST
(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
Comment 7 2013-11-06 07:37:12 PST
Created attachment 216174 [details] fixes after review
WebKit Commit Bot
Comment 8 2013-11-06 10:15:06 PST
Comment on attachment 216174 [details] fixes after review Clearing flags on attachment: 216174 Committed r158757: <http://trac.webkit.org/changeset/158757>
WebKit Commit Bot
Comment 9 2013-11-06 10:15:10 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.