WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2013-08-16 14:33:51 PDT
<
rdar://problem/14761152
>
Daniel Bates
Comment 2
2013-08-16 14:36:00 PDT
Created
attachment 208961
[details]
Patch
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
Committed
r154339
: <
http://trac.webkit.org/changeset/154339
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug