Summary: | AX: iOS: scrollable elements do not allow 3-finger swipe (with VoiceOver) | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Johnston <mj> | ||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cfleizach, commit-queue, darin, dbates, dmazzoni, jcraig, jdiggs, mario, samuel_white, simon.fraser, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | iPhone / iPad | ||||||||||
OS: | iOS 8.1 | ||||||||||
Attachments: |
|
Description
Michael Johnston
2015-02-23 00:03:18 PST
Created attachment 257143 [details]
patch
Created attachment 257185 [details]
patch
Created attachment 257204 [details]
patch
Comment on attachment 257204 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=257204&action=review Overall looks good to me (and I like the idea of moving that into AccessibilityObject, too). Setting r+ with just a few small comments below... > Source/WebCore/ChangeLog:15 > + (WebCore::AccessibilityObject::scrollToGlobalPoint): It does not look like you modified this > Source/WebCore/ChangeLog:21 > + (WebCore::AccessibilityObject::lastKnownIsIgnoredValue): Same for this one (not modified) > Source/WebCore/ChangeLog:24 > + (WebCore::AccessibilityObject::getScrollableAreaIfScrollable): > + (WebCore::AccessibilityObject::scrollTo): Ditto for these two. > Source/WebCore/accessibility/AccessibilityObject.cpp:2538 > + for (; scrollers.second && !(scrollers.first = scrollers.second->getScrollableAreaIfScrollable()); scrollers.second = scrollers.second->parentObject()) { } I find this line a bit hard to read. What about this: // Search up the parent chain until we find the first one that's scrollable. scrollers.first = nullptr; scrollers.second = parentObject(); while (!scrollers.first && scrollers.second) { scrollers.first = scrollers.second->getScrollableAreaIfScrollable()); scrollers.second = scrollers.second->parentObject()); } Or even: // Search up the parent chain until we find the first one that's scrollable. scrollers.first = nullptr; for (scrollers.second = parentObject(); !scrollers.first && scrollers.second; scrollers.second = scrollers.second->parentObject()) scrollers.first = scrollers.second->getScrollableAreaIfScrollable()); > Source/WebCore/accessibility/AccessibilityObject.cpp:2565 > + std::pair<ScrollableArea*, AccessibilityObject*> scrollers; > + scrollAreaAndAncestor(scrollers); > + if (!scrollers.first) It looks like you could put these three lines in a separate function returning a ScrollableArea*, and use it from this and the previous two functions (but not from scrollByPage, as you also need the .second element there) > Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:183 > - JSObjectCallAsFunction([mainFrame globalContext], m_notificationFunctionCallback, 0, 2, arguments, 0); > + JSObjectCallAsFunction([mainFrame globalContext], m_notificationFunctionCallback, 0, 3, arguments, 0); Nice catch |