RESOLVED FIXED 119914
[iOS] Upstream changes to WebCore/accessibility
https://bugs.webkit.org/show_bug.cgi?id=119914
Summary [iOS] Upstream changes to WebCore/accessibility
Daniel Bates
Reported 2013-08-16 14:33:35 PDT
Upstream iOS-specific changes to WebCore/accessibility.
Attachments
Patch (9.04 KB, patch)
2013-08-16 14:36 PDT, Daniel Bates
darin: review+
Radar WebKit Bug Importer
Comment 1 2013-08-16 14:33:51 PDT
Daniel Bates
Comment 2 2013-08-16 14:36:00 PDT
Daniel Bates
Comment 3 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.
chris fleizach
Comment 4 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
Darin Adler
Comment 5 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.
Daniel Bates
Comment 6 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.
Daniel Bates
Comment 7 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.
Daniel Bates
Comment 8 2013-08-20 10:30:09 PDT
Note You need to log in before you can comment on or make changes to this bug.