Bug 119914 - [iOS] Upstream changes to WebCore/accessibility
Summary: [iOS] Upstream changes to WebCore/accessibility
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-08-16 14:33 PDT by Daniel Bates
Modified: 2013-08-20 10:30 PDT (History)
10 users (show)

See Also:


Attachments
Patch (9.04 KB, patch)
2013-08-16 14:36 PDT, Daniel Bates
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2013-08-16 14:33:35 PDT
Upstream iOS-specific changes to WebCore/accessibility.
Comment 1 Radar WebKit Bug Importer 2013-08-16 14:33:51 PDT
<rdar://problem/14761152>
Comment 2 Daniel Bates 2013-08-16 14:36:00 PDT
Created attachment 208961 [details]
Patch
Comment 3 Daniel Bates 2013-08-16 14:37:31 PDT
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.
Comment 4 chris fleizach 2013-08-16 15:05:32 PDT
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
Comment 5 Darin Adler 2013-08-17 05:10:49 PDT
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.
Comment 6 Daniel Bates 2013-08-19 16:55:57 PDT
(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.
Comment 7 Daniel Bates 2013-08-20 10:15:38 PDT
(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.
Comment 8 Daniel Bates 2013-08-20 10:30:09 PDT
Committed r154339: <http://trac.webkit.org/changeset/154339>