Bug 158364 – AX: WKWebView should have API to prevent pinch-to-zoom always being allowed

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Nan Wang]

We should provide an API to developers so that they can set whether the WKWebView should always allow scaling of the web page, regardless of author intent.

<rdar://problem/26425468>

[Radar WebKit Bug Importer]

<rdar://problem/26631538>

[Nan Wang]

Created attachment 280472 [details]
initial patch

[Nan Wang]

Created attachment 280475 [details]
patch

fix build failure

[Nan Wang]

Created attachment 280478 [details]
patch

Another build fix for mac ports

[chris fleizach]

Comment on attachment 280478 [details]
patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:246
> + @seealso backForwardList.

This comment should be removed
@seealso backForwardList.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214
> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000

Is all the iOS versioning required? This code will never run on an older device right

> Tools/WebKitTestRunner/TestController.cpp:2144
> +#if PLATFORM(COCOA)

Should this be IOS

[Nan Wang]

Comment on attachment 280478 [details]
patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214
>> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
> 
> Is all the iOS versioning required? This code will never run on an older device right

Because we set the property WK_AVAILABLE(NA, 10_0), do you think we should remove that as well?

[chris fleizach]

Comment on attachment 280478 [details]
patch

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

>>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214
>>> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
>> 
>> Is all the iOS versioning required? This code will never run on an older device right
> 
> Because we set the property WK_AVAILABLE(NA, 10_0), do you think we should remove that as well?

It seems like that should be kept there so people compiling against different versions know whats there or not, but internally the versioning seems uncessary to me. Maybe someone else has a different opinion

[Tim Horton]

Comment on attachment 280478 [details]
patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:248
> +@property (nonatomic) BOOL alwaysUserScalable WK_AVAILABLE(NA, 10_0);

The naming of this API is surprising (and the fact that it's API at all, to start out with). I cc'd andersca for thoughts.

>>>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214
>>>> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
>>> 
>>> Is all the iOS versioning required? This code will never run on an older device right
>> 
>> Because we set the property WK_AVAILABLE(NA, 10_0), do you think we should remove that as well?
> 
> It seems like that should be kept there so people compiling against different versions know whats there or not, but internally the versioning seems uncessary to me. Maybe someone else has a different opinion

The versioning should definitely not be here, there's no reason for it and it absolutely can be run on different versions (and should be usable and testable there).

[Anders Carlsson]

(In reply to comment #8)
> Comment on attachment 280478 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280478&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:248
> > +@property (nonatomic) BOOL alwaysUserScalable WK_AVAILABLE(NA, 10_0);
> 
> The naming of this API is surprising (and the fact that it's API at all, to
> start out with). I cc'd andersca for thoughts.
> 
> >>>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214
> >>>> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
> >>> 
> >>> Is all the iOS versioning required? This code will never run on an older device right
> >> 
> >> Because we set the property WK_AVAILABLE(NA, 10_0), do you think we should remove that as well?

New APIs should be versioned. We use WK_IOS_TBA for new APIs. I don't like the name.

> > 
> > It seems like that should be kept there so people compiling against different versions know whats there or not, but internally the versioning seems uncessary to me. Maybe someone else has a different opinion
> 
> The versioning should definitely not be here, there's no reason for it and
> it absolutely can be run on different versions (and should be usable and
> testable there).

[Tim Horton]

(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 280478 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=280478&action=review
> > 
> > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:248
> > > +@property (nonatomic) BOOL alwaysUserScalable WK_AVAILABLE(NA, 10_0);
> > 
> > The naming of this API is surprising (and the fact that it's API at all, to
> > start out with). I cc'd andersca for thoughts.
> > 
> > >>>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214
> > >>>> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
> > >>> 
> > >>> Is all the iOS versioning required? This code will never run on an older device right
> > >> 
> > >> Because we set the property WK_AVAILABLE(NA, 10_0), do you think we should remove that as well?
> 
> New APIs should be versioned. We use WK_IOS_TBA for new APIs. I don't like
> the name.

The interface should have availability macros, yes, but the implementation shouldn't be #if'd out, should it?

[Nan Wang]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Comment on attachment 280478 [details]
> > > patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=280478&action=review
> > > 
> > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:248
> > > > +@property (nonatomic) BOOL alwaysUserScalable WK_AVAILABLE(NA, 10_0);
> > > 
> > > The naming of this API is surprising (and the fact that it's API at all, to
> > > start out with). I cc'd andersca for thoughts.
> > > 
> > > >>>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214
> > > >>>> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
> > > >>> 
> > > >>> Is all the iOS versioning required? This code will never run on an older device right
> > > >> 
> > > >> Because we set the property WK_AVAILABLE(NA, 10_0), do you think we should remove that as well?
> > 
> > New APIs should be versioned. We use WK_IOS_TBA for new APIs. I don't like
> > the name.

Do you have any suggestion for the name?

> 
> The interface should have availability macros, yes, but the implementation
> shouldn't be #if'd out, should it?

I think the code won't compile if the version is lower than the availability macro.

[Tim Horton]

(In reply to comment #11)
> > The interface should have availability macros, yes, but the implementation
> > shouldn't be #if'd out, should it?
> 
> I think the code won't compile if the version is lower than the availability
> macro.

No. The availability macro (WK_AVAILABLE) is unrelated to what code actually builds, and is just an annotation. You only want the availability macro, not the #if __IPHONE_OS_VERSION_MIN_REQUIRED bits.

[Nan Wang]

(In reply to comment #12)
> No. The availability macro (WK_AVAILABLE) is unrelated to what code actually
> builds, and is just an annotation. You only want the availability macro, not
> the #if __IPHONE_OS_VERSION_MIN_REQUIRED bits.

Got it. But why in my previous patch, I only have the (WK_AVAILABLE) macro, but it gave me the error: property 'alwaysUserScalable' not found on object of type 'WKWebView *' on the mac port?

[Tim Horton]

(In reply to comment #13)
> (In reply to comment #12)
> > No. The availability macro (WK_AVAILABLE) is unrelated to what code actually
> > builds, and is just an annotation. You only want the availability macro, not
> > the #if __IPHONE_OS_VERSION_MIN_REQUIRED bits.
> 
> Got it. But why in my previous patch, I only have the (WK_AVAILABLE) macro,
> but it gave me the error: property 'alwaysUserScalable' not found on object
> of type 'WKWebView *' on the mac port?

If your property only exists/works on iOS you may need some #if PLATFORM(IOS) guards, but not the version guards.

[Nan Wang]

(In reply to comment #14)
> If your property only exists/works on iOS you may need some #if
> PLATFORM(IOS) guards, but not the version guards.

Ok, will remove the #if __IPHONE_OS_VERSION_MIN_REQUIRED

[Nan Wang]

Created attachment 280860 [details]
patch

- Addressed review comments.
- removed the __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 check
- Not sure what a good name would be for the property. Suggestions welcome!

[Build Bot]

Comment on attachment 280860 [details]
patch

Attachment 280860 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1469705

New failing tests:
editing/selection/selection-in-iframe-removed-crash.html

[Build Bot]

Created attachment 280867 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[chris fleizach]

(In reply to comment #17)
> Comment on attachment 280860 [details]
> patch
> 
> Attachment 280860 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/1469705
> 
> New failing tests:
> editing/selection/selection-in-iframe-removed-crash.html

Nan is this failure related to this patch

[Nan Wang]

(In reply to comment #19)
> (In reply to comment #17)
> > Comment on attachment 280860 [details]
> > patch
> > 
> > Attachment 280860 [details] did not pass mac-debug-ews (mac):
> > Output: http://webkit-queues.webkit.org/results/1469705
> > 
> > New failing tests:
> > editing/selection/selection-in-iframe-removed-crash.html
> 
> Nan is this failure related to this patch

I don't think so.

[chris fleizach]

Comment on attachment 280860 [details]
patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:247
> +@property (nonatomic) BOOL alwaysUserScalable WK_AVAILABLE(NA, WK_IOS_TBA);

How do people feel about "ignoresUserScalable"

[Nan Wang]

Created attachment 283048 [details]
Patch

- Changed the property name to ignoresViewportScaleLimits
- Moved the property to WKWebViewConfiguration
- Added a test option so that we can set the configuration in test.

[Anders Carlsson]

Comment on attachment 283048 [details]
Patch

Instead of having anything on the WKWebView, just add a boolean to WebPageCreationParameters and pipe it through from the WKWebView initializer. I think having a setting on the Internals object for testing is better than having an injected bundle function.

[Nan Wang]

Created attachment 283255 [details]
patch

Based on the review:
- Added a boolean in WebPageCreationParameters and removed logics in WKWebView
- Since the viewport configuration is in WebKit2 and Internals is in WebCore, there's no easy way to communicate between. So I kept the injected bundle function.

[Nan Wang]

Created attachment 283256 [details]
patch

fixed a small typo

[Build Bot]

Comment on attachment 283256 [details]
patch

Attachment 283256 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1652076

New failing tests:
fast/scrolling/ios/scrollTo-at-page-load.html

[Build Bot]

Created attachment 283257 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[Nan Wang]

Created attachment 283262 [details]
patch

The failure test passed on my local machine. Let's try again.

[WebKit Commit Bot]

Comment on attachment 283262 [details]
patch

Clearing flags on attachment: 283262

Committed r203075: <http://trac.webkit.org/changeset/203075>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.