Bug 130071

Summary: AX: AccessibilityObject::invalidStatus() is incorrect when aria-invalid="undefined" or whitespace
Product: WebKit Reporter: James Craig <jcraig>
Component: AccessibilityAssignee: James Craig <jcraig>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, jdiggs, mario, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
none
patch
none
patch
none
patch with review feedback
none
patch merges conflicts with TOT
cfleizach: review+
patch to fix ATK build failure
none
previous patch -virtual none

Description James Craig 2014-03-11 00:25:17 PDT
AX: AccessibilityObject::invalidStatus() is incorrect when aria-invalid="undefined"

The when the string value "undefined" is used explicitly, it should be treated the same as ariaInvalid.isEmpty()

    // If 'false', empty or not present, it should return false.
    if (ariaInvalid.isEmpty() || equalIgnoringCase(ariaInvalid, invalidStatusFalse))
        return invalidStatusFalse;

Should be:

    // If 'false', empty or not present, it should return false.
    if (ariaInvalid.isEmpty() || equalIgnoringCase(ariaInvalid, invalidStatusFalse) || equalIgnoringCase(ariaInvalid, "undefined"))
        return invalidStatusFalse;
Comment 1 James Craig 2014-03-13 19:12:57 PDT
<rdar://problem/16322420>
Comment 2 James Craig 2014-03-13 19:17:10 PDT
Created attachment 226639 [details]
patch
Comment 3 James Craig 2014-03-13 19:30:02 PDT
[ 16%] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/inspector/InspectorResourceAgent.cpp.o
/mnt/eflews/git/webkit/Source/WebCore/accessibility/AccessibilityObject.cpp: In member function 'virtual const WTF::AtomicString& WebCore::AccessibilityObject::invalidStatus() const':
/mnt/eflews/git/webkit/Source/WebCore/accessibility/AccessibilityObject.cpp:1540:103: error: 'stripLeadingAndTrailingHTMLSpaces' was not declared in this scope
make[2]: *** [Source/WebCore/CMakeFiles/WebCore.dir/accessibility/AccessibilityObject.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
Comment 4 James Craig 2014-03-13 19:33:38 PDT
Created attachment 226640 [details]
patch
Comment 5 James Craig 2014-03-13 20:07:03 PDT
Created attachment 226643 [details]
patch
Comment 6 chris fleizach 2014-03-14 09:24:39 PDT
Comment on attachment 226643 [details]
patch

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

> Source/WebCore/ChangeLog:3
> +        AX: AccessibilityObject::invalidStatus() is incorrect when aria-invalid="undefined" or whitespace

i don't see undefined listed in the spec
http://www.w3.org/TR/wai-aria/states_and_properties

> Source/WebCore/accessibility/AccessibilityObject.cpp:1534
> +    DEFINE_STATIC_LOCAL(const AtomicString, invalidStatusGrammar, ("grammar", AtomicString::ConstructFromLiteral));

there's been a desire to stop using statics for things like this. so we might as well change this style if we're going to modify this code.

Let's just use

ASCIILiteral("grammar") when necessary.
See
  https://chromium.googlesource.com/chromium/blink/+/873a4a1f950d7cd7ebe27baba2c0e7e9491190f4%5E%21/#F3
for more examples
Comment 7 James Craig 2014-03-14 10:09:57 PDT
Comment on attachment 226643 [details]
patch

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

>> Source/WebCore/ChangeLog:3
>> +        AX: AccessibilityObject::invalidStatus() is incorrect when aria-invalid="undefined" or whitespace
> 
> i don't see undefined listed in the spec
> http://www.w3.org/TR/wai-aria/states_and_properties

From http://www.w3.org/WAI/PF/aria/complete#propcharacteristic_value

The "undefined" value, when allowed on a state or property, is an explicit indication that the state or property is not set. The value is used on states and properties that support tokens, and the "undefined" value is a string that is one of the allowed tokens.
Comment 8 James Craig 2014-03-14 11:31:03 PDT
Created attachment 226739 [details]
patch with review feedback
Comment 9 James Craig 2014-03-14 11:45:20 PDT
Created attachment 226743 [details]
patch merges conflicts with TOT
Comment 10 chris fleizach 2014-03-14 11:48:31 PDT
Comment on attachment 226743 [details]
patch merges conflicts with TOT

we should remove virtual from the method, since subclassers won't override this
Comment 11 James Craig 2014-03-14 11:58:18 PDT
Created attachment 226746 [details]
patch to fix ATK build failure
Comment 12 James Craig 2014-03-14 13:19:43 PDT
Created attachment 226756 [details]
previous patch -virtual
Comment 13 WebKit Commit Bot 2014-03-14 14:27:19 PDT
Comment on attachment 226756 [details]
previous patch -virtual

Rejecting attachment 226756 [details] from commit-queue.

jcraig@apple.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 14 James Craig 2014-03-14 14:27:50 PDT
Told you, Sam. ;-)
Comment 15 WebKit Commit Bot 2014-03-14 15:02:32 PDT
Comment on attachment 226756 [details]
previous patch -virtual

Clearing flags on attachment: 226756

Committed r165656: <http://trac.webkit.org/changeset/165656>
Comment 16 WebKit Commit Bot 2014-03-14 15:02:36 PDT
All reviewed patches have been landed.  Closing bug.