Bug 119914 – [iOS] Upstream changes to WebCore/accessibility

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Daniel Bates]

Upstream iOS-specific changes to WebCore/accessibility.

[Radar WebKit Bug Importer]

<rdar://problem/14761152>

[Daniel Bates]

Created attachment 208961 [details]
Patch

[Daniel Bates]

We should consider making AccessibilityObject::headingElementForNode() available to all ports or move it into an iOS-specific file. I suggest we do this in a separate bug.

[chris fleizach]

Comment on attachment 208961 [details]
Patch

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

thanks for working on this

> Source/WebCore/accessibility/AccessibilityMenuList.cpp:92
>      return !static_cast<RenderMenuList*>(m_renderer)->popupIsVisible();

does RenderMenuList not exist on iOS?

> Source/WebCore/accessibility/AccessibilityObject.cpp:1230
> +AccessibilityObject* AccessibilityObject::headingElementForNode(Node* node)

i agree, i think we should just remove the PLATFORM(IOS) flag here. I don't think we need a separate bug necessary

> Source/WebCore/accessibility/AccessibilityObject.h:875
> +    int accessibilityPasswordFieldLength();

this should probably be fixed so that it's called
passwordTextLength() and returns an unsigned.

i also don't see this in the AXObject.cpp file

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:731
>      if (m_renderer->isFileUploadControl())

i thought we had file upload controls on iOS now

[Darin Adler]

Comment on attachment 208961 [details]
Patch

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

It seems fine to upstream this as-is. We can also later fix the many problems in this code.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1334
> +    DEFINE_STATIC_LOCAL(const String, noAction, ());
> +    return noAction;

This should just be:

    return nullAtom();

Or maybe:

    return nullAtom().string();

No need for this extra local.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3177
> +    DEFINE_STATIC_LOCAL(const String, noAction, ());
> +    return noAction;

This should just be:

    return nullAtom();

Or maybe:

    return nullAtom().string();

No need for this extra local.

[Daniel Bates]

(In reply to comment #4)
> (From update of attachment 208961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208961&action=review
> 
> thanks for working on this
> 
> > Source/WebCore/accessibility/AccessibilityMenuList.cpp:92
> >      return !static_cast<RenderMenuList*>(m_renderer)->popupIsVisible();
> 
> does RenderMenuList not exist on iOS?

It does. However we don't define RenderMenuList::popupIsVisible() on iOS.

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1230
> > +AccessibilityObject* AccessibilityObject::headingElementForNode(Node* node)
> 
> i agree, i think we should just remove the PLATFORM(IOS) flag here. I don't think we need a separate bug necessary

Will fix before landing.

> 
> > Source/WebCore/accessibility/AccessibilityObject.h:875
> > +    int accessibilityPasswordFieldLength();
> 
> this should probably be fixed so that it's called
> passwordTextLength() and returns an unsigned.

I agree. Moreover, the implementation for this method can be simplified: <https://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm?rev=152149#L61>. I will look to change the data type of the return value and cleanup the implementation of this method in a follow bug as the purpose of this bug is to upstream iOS-specific changes to the accessibility code.

> 
> i also don't see this in the AXObject.cpp file

The method accessibilityPasswordFieldLength() is defined in file Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm: <https://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm?rev=152149>.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:731
> >      if (m_renderer->isFileUploadControl())
> 
> i thought we had file upload controls on iOS now

We do! Will fix before landing. For completeness, this code is commented out in the iOS WebKit source code repository. Filed <rdar://problem/14778945> to fix this.

[Daniel Bates]

(In reply to comment #5)
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1334
> > +    DEFINE_STATIC_LOCAL(const String, noAction, ());
> > +    return noAction;
> 
> This should just be:
> 
>     return nullAtom();
> 
> Or maybe:
> 
>     return nullAtom().string();
> 
> No need for this extra local.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3177
> > +    DEFINE_STATIC_LOCAL(const String, noAction, ());
> > +    return noAction;
> 
> This should just be:
> 
>     return nullAtom();
> 
> Or maybe:
> 
>     return nullAtom().string();
> 
> No need for this extra local.

I'll fix these issues in bug #120072 because we can also perform a similar substitution in the non-iOS-specific portion of these functions.

[Daniel Bates]

Committed r154339: <http://trac.webkit.org/changeset/154339>