Bug 132028 - AX: WK2: iOS web page scrolling doesn't work with VoiceOver
Summary: AX: WK2: iOS web page scrolling doesn't work with VoiceOver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-22 16:26 PDT by chris fleizach
Modified: 2014-05-02 09:20 PDT (History)
11 users (show)

See Also:


Attachments
patch (8.55 KB, patch)
2014-04-22 16:28 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (8.53 KB, patch)
2014-04-22 16:32 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (8.74 KB, patch)
2014-04-22 16:34 PDT, chris fleizach
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (461.13 KB, application/zip)
2014-04-22 17:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (507.82 KB, application/zip)
2014-04-22 18:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (533.27 KB, application/zip)
2014-04-22 18:58 PDT, Build Bot
no flags Details
patch (8.26 KB, patch)
2014-04-23 13:55 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (8.69 KB, patch)
2014-04-24 10:16 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (8.62 KB, patch)
2014-05-01 16:30 PDT, chris fleizach
mario: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2014-04-22 16:26:47 PDT
Using 3 finger scroll doesn't work with WK2
Comment 1 Radar WebKit Bug Importer 2014-04-22 16:26:55 PDT
<rdar://problem/16693253>
Comment 2 chris fleizach 2014-04-22 16:28:41 PDT
Created attachment 229920 [details]
patch
Comment 3 WebKit Commit Bot 2014-04-22 16:29:36 PDT
Attachment 229920 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1032:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 chris fleizach 2014-04-22 16:32:33 PDT
Created attachment 229921 [details]
patch
Comment 5 chris fleizach 2014-04-22 16:34:01 PDT
Created attachment 229922 [details]
patch
Comment 6 Build Bot 2014-04-22 17:31:34 PDT
Comment on attachment 229922 [details]
patch

Attachment 229922 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6209446032179200

New failing tests:
platform/mac/accessibility/scroll-to-visible-action.html
Comment 7 Build Bot 2014-04-22 17:31:37 PDT
Created attachment 229929 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 8 Build Bot 2014-04-22 18:04:16 PDT
Comment on attachment 229922 [details]
patch

Attachment 229922 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5877341980983296

New failing tests:
fast/events/ghostly-mousemoves-in-subframe.html
platform/mac/accessibility/scroll-to-visible-action.html
platform/mac/accessibility/search-predicate.html
Comment 9 Build Bot 2014-04-22 18:04:20 PDT
Created attachment 229934 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 10 Build Bot 2014-04-22 18:58:01 PDT
Comment on attachment 229922 [details]
patch

Attachment 229922 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6231144206958592

New failing tests:
fast/events/ghostly-mousemoves-in-subframe.html
platform/mac/accessibility/scroll-to-visible-action.html
platform/mac/accessibility/search-predicate.html
Comment 11 Build Bot 2014-04-22 18:58:05 PDT
Created attachment 229940 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 12 chris fleizach 2014-04-23 13:55:35 PDT
Created attachment 230004 [details]
patch
Comment 13 Simon Fraser (smfr) 2014-04-23 17:26:00 PDT
Comment on attachment 230004 [details]
patch

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

> Source/WebCore/ChangeLog:8
> +        Now that the scroll view is in the WebProcess, WebCore needs to implement accessibilityScroll:

You mean ScrollView, not UIScrollView I presume? Maybe say "ScrollView" here.

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:62
> +#import <UIKit/UIAccessibility.h>

We don't want WebCore to depend on UIKit. Is this just for the enum values? Can you just redeclare them?
Comment 14 chris fleizach 2014-04-23 17:28:25 PDT
(In reply to comment #13)
> (From update of attachment 230004 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230004&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Now that the scroll view is in the WebProcess, WebCore needs to implement accessibilityScroll:
> 
> You mean ScrollView, not UIScrollView I presume? Maybe say "ScrollView" here.
> 
> > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:62
> > +#import <UIKit/UIAccessibility.h>
> 
> We don't want WebCore to depend on UIKit. Is this just for the enum values? Can you just redeclare them?

Yes I can do that

Thanks
Comment 15 Darin Adler 2014-04-24 09:07:29 PDT
Comment on attachment 230004 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1405
> +            scrollView = toAccessibilityScrollView(scrollParent)->scrollView();

I think this should be a return rather than using a local variable.
Comment 16 chris fleizach 2014-04-24 10:16:46 PDT
Created attachment 230091 [details]
patch
Comment 17 Mario Sanchez Prada 2014-04-29 06:00:23 PDT
Comment on attachment 230091 [details]
patch

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

The accessibility bits sound good to me and the IOS part looks (with my limited knowledge of Obj-C) almost good(*) as well, although I'd rather have someone else with a better understanding double checking it.

(*) I'm not sure, but it looks to me like two cases could be swapped, see my comment below...

> Source/WebCore/accessibility/AccessibilityObject.cpp:1405
> +            return toAccessibilityScrollView(scrollParent)->scrollView();
> +            break;

You only need the return line here, not the break

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1057
> +    case UIAccessibilityScrollDirectionUp: {
> +        int scrollAmount = scrollVisibleRect.size().height();
> +        int newY = scrollPosition.y() - scrollAmount;
> +        newScrollPosition.setY(std::max(newY, 0));
> +        break;
> +    }

Shouldn't this block be for the UIAccessibilityScrollDirectionDown case? And in case it's correct as it is now, is then the Right/Left cases correct?

My question comes from the fact that I would expect Right to be consistent with Down (and Up being consistent to left) in the operations they do with X/Y and scrollamount (either add or substract), assuming that the (0,0) coordinate is in the NW corner.

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1063
> +        

Extra blank line
Comment 18 chris fleizach 2014-05-01 16:20:11 PDT
(In reply to comment #17)
> (From update of attachment 230091 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230091&action=review
> 
> The accessibility bits sound good to me and the IOS part looks (with my limited knowledge of Obj-C) almost good(*) as well, although I'd rather have someone else with a better understanding double checking it.
> 
> (*) I'm not sure, but it looks to me like two cases could be swapped, see my comment below...
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1405
> > +            return toAccessibilityScrollView(scrollParent)->scrollView();
> > +            break;
> 
> You only need the return line here, not the break
> 
> > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1057
> > +    case UIAccessibilityScrollDirectionUp: {
> > +        int scrollAmount = scrollVisibleRect.size().height();
> > +        int newY = scrollPosition.y() - scrollAmount;
> > +        newScrollPosition.setY(std::max(newY, 0));
> > +        break;
> > +    }
> 
> Shouldn't this block be for the UIAccessibilityScrollDirectionDown case? And in case it's correct as it is now, is then the Right/Left cases correct?
> 

Thanks for reviewing.

I've double-checked here and confirmed that we have the correct directionality. 

So for example the "up" case, means we want to move the content "up" in its scroll view, because the VO user has done a 3 finger scroll upwards. That means they expect the beginning of the content to become visible.

So in that case we take the current y position and remove scrollAmount from it, getting it closer to showing the beginning portions of the content.


> My question comes from the fact that I would expect Right to be consistent with Down (and Up being consistent to left) in the operations they do with X/Y and scrollamount (either add or substract), assuming that the (0,0) coordinate is in the NW corner.
> 
> > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1063
> > +        
> 
> Extra blank line
Comment 19 chris fleizach 2014-05-01 16:30:53 PDT
Created attachment 230627 [details]
patch

Slightly updated patch. When making the new scroll offset, I forgot to initialize that scrollOffset with the current offset.
(also mario's comments)
Comment 20 chris fleizach 2014-05-02 09:20:18 PDT
http://trac.webkit.org/changeset/168171

Thanks mario!