Bug 99502

Created attachment 169116 [details] patch

Comment on attachment 169116 [details] patch Attachment 169116 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14395502 New failing tests: accessibility/loading-iframe-sends-notification.html

Hard to believe that accessibility/loading-iframe-sends-notification.html is failing because of this patch. It does run fine on Mac and this patch shouldn't affect other platforms.

Comment on attachment 169116 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=169116&action=review I like the idea, since it should make it easier for ports to "select" which accessible textual information is interested in in any case, without having to reinvent the wheel. Besides, I've informally reviewed the patch and it mostly looks good to me, although it would be worth to try to understand why it made that layout test fail in chromium EWS. I just have some really minor comments probably a bit too subjective sometimes. See them below > Source/WebCore/ChangeLog:12 > + This change categorizes and orders accessibility text based on WAI-ARIA text computation rules and then > + allows the platform (only Mac right now) to decide how best to apply that text to its own AX API. A link to the part of the WAI-ARIA specification where these rules are explained might be useful here. > Source/WebCore/ChangeLog:72 > + (WebCore::localizedMediaControlElementHelpText): I know it's a pain, but in theory we should be adding a short description to every function in the ChangeLog > Source/WebCore/accessibility/AccessibilityObject.h:546 > + // Accessibility Text - (To be deprecated). > + virtual String accessibilityDescription() const { return String(); } > + virtual String title() const { return String(); } > + virtual String helpText() const { return String(); } I understand that other ports (e.g. GTK) should migrate the usage of these functions to the new AccessibilityText API, right? > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1096 > + Extra line > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1107 > + titleElementText(textOrder); > + > + alternativeText(textOrder); > + > + visibleText(textOrder); > + > + helpText(textOrder); I would probably compact these without extra lines. Also, I would try to keep them in the same order as they are declared in the header file: void alternativeText(Vector<AccessibilityText>&) const; void visibleText(Vector<AccessibilityText>&) const; void titleElementText(Vector<AccessibilityText>&); void helpText(Vector<AccessibilityText>&) const; String alternativeTextForWebArea() const; void ariaLabeledByText(Vector<AccessibilityText>&) const; Additionally I would probably reorder the definitions for these functions in this same file to be consistent too with that order. I think it would make the code easier to read. Just a suggestion, not a big deal anyway as it's just a matter of taster > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1251 > + if (isHeading() || isLink()) > + useTextUnderElement = true; > + > + // If it's focusable but it's not content editable or a known control type, then it will appear to > + // the user as a single atomic object, so we should use its text as the default title. > + if (isGenericFocusableElement()) > + useTextUnderElement = true; Would it make sense to merge these two ifs in a single block? > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1258 > + Extra line

(In reply to comment #3) > Hard to believe that > accessibility/loading-iframe-sends-notification.html > > is failing because of this patch. It does run fine on Mac and this patch shouldn't affect other platforms. I think the test may be too brittle. Mark it as skipped and assign the bug to me, with a note to take a look after this bug lands. - Dominic

Created attachment 169318 [details] patch