Bug 72754 – Send an AXCheckedStateChanged notification when the aria-checked attribute changes.

• Component: New Bugs
• Status: RESOLVED FIXED

[David Tseng]

Send an AXCheckedStateChanged notification when the aria-checked attribute changes.

[David Tseng]

Created attachment 115868 [details]
Patch

[David Tseng]

r?

[WebKit Review Bot]

Attachment 115868 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Tseng]

Created attachment 115872 [details]
Patch

[chris fleizach]

Comment on attachment 115872 [details]
Patch

need to add a layout test.
also i think we should have a checkedStateChanged() method in axObjectCache() so that we don't expose the postNotification mechanism as much outside of accessibility code.

[David Tseng]

Created attachment 117061 [details]
Patch

[chris fleizach]

Comment on attachment 117061 [details]
Patch

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

this test will likely fail on mac, windows and gtk so you should ask this test to those Skipped lists

> Source/WebCore/dom/Element.cpp:778
>      } else if (attrName == aria_selectedAttr)

no brackets for the else if since it's a one liner

> LayoutTests/accessibility/aria-checkbox-sends-notification.html:18
> +                return;

no need to special case out AXLayoutComplete.
Just check that CheckedStateChange has been seen two times.

> LayoutTests/accessibility/aria-checkbox-sends-notification.html:20
> +

notificationCount++ should suffice (instead of += 1)

> LayoutTests/accessibility/aria-checkbox-sends-notification.html:26
> +

no need to add a test complete yourself. the js-test-post should do that

> LayoutTests/accessibility/aria-checkbox-sends-notification.html:41
> +        }, 10);

i would remove this block. the test will timeout anyway if it doesn't finish, and this block may mask a true failure
if we're not receiving the notifications something is wrong

> LayoutTests/accessibility/aria-checkbox-sends-notification.html:47
> +        }, false);

there's no need to do this through an addEventListener. you can just call runTest() and it should work just fine

> LayoutTests/accessibility/aria-checkbox-sends-notification.html:57
> +

you're missing the js-test-post include here

[David Tseng]

Created attachment 117202 [details]
Patch

[Early Warning System Bot]

Comment on attachment 117202 [details]
Patch

Attachment 117202 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10693224

[chris fleizach]

qt build failure

/usr/bin/gold: /storage/WebKit-qt-ews/WebKitBuild/Release/Source/WebCore/release/libwebcore.a(Element.o): in function WebCore::Element::updateAfterAttributeChanged(WebCore::Attribute*):Element.cpp(.text._ZN7WebCore7Element27updateAfterAttributeChangedEPNS_9AttributeE+0xd0): error: undefined reference to 'WebCore::AXObjectCache::checkedStateChanged(WebCore::RenderObject*)'
/usr/bin/gold: /storage/WebKit-qt-ews/WebKitBuild/Release/Source/WebCore/release/libwebcore.a(HTMLInputElement.o): in function WebCore::HTMLInputElement::setChecked(bool, bool):HTMLInputElement.cpp(.text._ZN7WebCore16HTMLInputElement10setCheckedEbb+0xc7): error: undefined reference to 'WebCore::AXObjectCache::checkedStateChanged(WebCore::RenderObject*)'

[Gyuyoung Kim]

Comment on attachment 117202 [details]
Patch

Attachment 117202 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10701082

[David Tseng]

Created attachment 117206 [details]
Patch

[David Tseng]

r?

[David Tseng]

cq?

[chris fleizach]

There's no flag requesting anything for cq in the cq field

[WebKit Review Bot]

Comment on attachment 117206 [details]
Patch

Clearing flags on attachment: 117206

Committed r101855: <http://trac.webkit.org/changeset/101855>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.