Bug 158364 - AX: WKWebView should have API to prevent pinch-to-zoom always being allowed
Summary: AX: WKWebView should have API to prevent pinch-to-zoom always being allowed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-06-03 15:04 PDT by Nan Wang
Modified: 2016-07-11 12:09 PDT (History)
11 users (show)

See Also:


Attachments
initial patch (26.13 KB, patch)
2016-06-03 15:19 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (26.90 KB, patch)
2016-06-03 15:32 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (27.19 KB, patch)
2016-06-03 16:00 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (26.31 KB, patch)
2016-06-08 17:47 PDT, Nan Wang
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (1.42 MB, application/zip)
2016-06-08 18:53 PDT, Build Bot
no flags Details
Patch (31.30 KB, patch)
2016-07-07 14:20 PDT, Nan Wang
andersca: review-
Details | Formatted Diff | Diff
patch (33.80 KB, patch)
2016-07-09 03:52 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (33.80 KB, patch)
2016-07-09 03:58 PDT, Nan Wang
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (613.21 KB, application/zip)
2016-07-09 05:08 PDT, Build Bot
no flags Details
patch (33.79 KB, patch)
2016-07-09 10:54 PDT, Nan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2016-06-03 15:04:30 PDT
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>
Comment 1 Radar WebKit Bug Importer 2016-06-03 15:07:39 PDT
<rdar://problem/26631538>
Comment 2 Nan Wang 2016-06-03 15:19:04 PDT
Created attachment 280472 [details]
initial patch
Comment 3 Nan Wang 2016-06-03 15:32:53 PDT
Created attachment 280475 [details]
patch

fix build failure
Comment 4 Nan Wang 2016-06-03 16:00:23 PDT
Created attachment 280478 [details]
patch

Another build fix for mac ports
Comment 5 chris fleizach 2016-06-08 14:49:44 PDT
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
Comment 6 Nan Wang 2016-06-08 14:56:06 PDT
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?
Comment 7 chris fleizach 2016-06-08 14:57:29 PDT
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
Comment 8 Tim Horton 2016-06-08 15:47:36 PDT
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).
Comment 9 Anders Carlsson 2016-06-08 15:48:45 PDT
(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).
Comment 10 Tim Horton 2016-06-08 15:50:40 PDT
(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?
Comment 11 Nan Wang 2016-06-08 16:01:35 PDT
(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.
Comment 12 Tim Horton 2016-06-08 16:13:54 PDT
(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.
Comment 13 Nan Wang 2016-06-08 16:18:37 PDT
(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?
Comment 14 Tim Horton 2016-06-08 16:48:35 PDT
(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.
Comment 15 Nan Wang 2016-06-08 16:54:02 PDT
(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
Comment 16 Nan Wang 2016-06-08 17:47:06 PDT
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!
Comment 17 Build Bot 2016-06-08 18:53:52 PDT
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
Comment 18 Build Bot 2016-06-08 18:53:56 PDT
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
Comment 19 chris fleizach 2016-06-14 10:58:33 PDT
(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
Comment 20 Nan Wang 2016-06-14 11:11:53 PDT
(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.
Comment 21 chris fleizach 2016-06-14 15:22:24 PDT
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"
Comment 22 Nan Wang 2016-07-07 14:20:12 PDT
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.
Comment 23 Anders Carlsson 2016-07-08 12:22:31 PDT
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.
Comment 24 Nan Wang 2016-07-09 03:52:06 PDT
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.
Comment 25 Nan Wang 2016-07-09 03:58:22 PDT
Created attachment 283256 [details]
patch

fixed a small typo
Comment 26 Build Bot 2016-07-09 05:08:18 PDT
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
Comment 27 Build Bot 2016-07-09 05:08:22 PDT
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
Comment 28 Nan Wang 2016-07-09 10:54:23 PDT
Created attachment 283262 [details]
patch

The failure test passed on my local machine. Let's try again.
Comment 29 WebKit Commit Bot 2016-07-11 12:09:09 PDT
Comment on attachment 283262 [details]
patch

Clearing flags on attachment: 283262

Committed r203075: <http://trac.webkit.org/changeset/203075>
Comment 30 WebKit Commit Bot 2016-07-11 12:09:15 PDT
All reviewed patches have been landed.  Closing bug.