We should plumb through the accessibility setting to WebKit in order to make user-scalable enable/disable on the fly.
<rdar://problem/24988193>
Created attachment 273065 [details] patch
Created attachment 273066 [details] patch
Comment on attachment 273066 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273066&action=review > Source/WebCore/ChangeLog:10 > + No new tests. we should probably add a test that override force scale (which can be set with Settings in the layout test) also overrides max scale > Source/WebCore/page/ViewportConfiguration.h:91 > + double maximumScale() const { return m_forceAlwaysUserScalable ? 10.0 : m_configuration.maximumScale; } we should probably define this is a const double somewhere > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:214 > +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 #if PLATFORM(IOS) && > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:215 > +#include <WebCore/SoftLinking.h> i think you need another #if #if __has_include(<AccessibilitySupport.h>) #else extern "C" { Boolean _AXSForceAllowWebScaling(); CFStringRef kAXSAllowForceWebScalingEnabledNotification; } #endif so that this can be built by other people take a look at WeakObjCPtr.h > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:602 > + CFNotificationCenterRemoveObserver(CFNotificationCenterGetLocalCenter(), this, 0, 0); nullptr, nullptr > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2820 > + m_viewportConfiguration.setForceAlwaysUserScalable(_AXSForceAllowWebScaling()); it seems like this should also respect store.getBoolValueForKey(WebPreferencesKey::forceAlwaysUserScalableKey()); maybe this method should have all the logic and then updatePreferences below calls updateForceAlways..()
Comment on attachment 273066 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273066&action=review Will address other comments. >> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:214 >> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > > #if PLATFORM(IOS) && the above includes are already in #if PLATFORM(IOS) >> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2820 >> + m_viewportConfiguration.setForceAlwaysUserScalable(_AXSForceAllowWebScaling()); > > it seems like this should also respect > store.getBoolValueForKey(WebPreferencesKey::forceAlwaysUserScalableKey()); > > maybe this method should have all the logic and then updatePreferences below calls updateForceAlways..() In this case we should cache store.getBoolValueForKey(WebPreferencesKey::forceAlwaysUserScalableKey()) somewhere? When we get a notification we don't have access to the store object.
Created attachment 273127 [details] patch For the layout test setting, do you mean InternalSettings? I'm not sure if we should expose the ViewportConfiguration so that we can override the force scale? And we should make this a WebKit2 only test I guess.
Attachment 273127 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:223: _AXSForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [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 on attachment 273127 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273127&action=review > Source/WebCore/ChangeLog:9 > + is it possible to make a test for this > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:224 > +CFStringRef kAXSAllowForceWebScalingEnabledNotification; did you check can this be built for simulator when <AccessibilitySupport.h> is not available (essentially using the public SDK)
Created attachment 273215 [details] patch Added a layout test. Also checked that without AccessibilitySupport.h we can still build for simulator.
Attachment 273215 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:223: _AXSForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 273215 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273215&action=review > Source/WebCore/ChangeLog:8 > + Overridden the maximum scale factor when forceAlwaysUserScalable is true. Overridden -> Overide > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:609 > +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 seems like we don't need the PLATFORM(IOS) part
Created attachment 273216 [details] patch addressed review comments
Attachment 273216 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:223: _AXSForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build failures on gtk, working on it.
Created attachment 273234 [details] build fix patch Build fix. Make ViewportConfiguration test only on iOS.
Attachment 273234 [details] did not pass style-queue: ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:223: _AXSForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 273234 [details] build fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=273234&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:215 > +#if __has_include(<AccessibilitySupport.h>) we might not need to do this difference because we can't use anything in the import directly. everything is a symbol that needs to be soft linked in > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:222 > +extern "C" { does this compile and run? it seems like we still probably need to soft link libAX in this case
Created attachment 273237 [details] patch review comments
Created attachment 273244 [details] patch build fix
Comment on attachment 273244 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273244&action=review > Source/WebCore/testing/Internals.cpp:3500 > +#if PLATFORM(IOS) use UNUSED_PARAM(forceAlwaysUserScalableEnabled);
Created attachment 273246 [details] patch review comments
Created attachment 273249 [details] patch more build fix. Guess we still need "if PLATFORM(IOS)" in ~WebPage()
Comment on attachment 273249 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273249&action=review > Source/WebCore/testing/Internals.cpp:3501 > + m_viewportConfiguration.setForceAlwaysUserScalable(forceAlwaysUserScalableEnabled); Is there a reason we need to make this stuff iOS only?
Comment on attachment 273249 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273249&action=review >> Source/WebCore/testing/Internals.cpp:3501 >> + m_viewportConfiguration.setForceAlwaysUserScalable(forceAlwaysUserScalableEnabled); > > Is there a reason we need to make this stuff iOS only? Do we have user scalable feature on Mac? If yes I can add support for Mac too.
Created attachment 273273 [details] Support the Internals setting on Mac
Comment on attachment 273273 [details] Support the Internals setting on Mac View in context: https://bugs.webkit.org/attachment.cgi?id=273273&action=review > Source/WebCore/testing/Internals.cpp:3500 > +#if PLATFORM(IOS) || PLATFORM(MAC) why not support this for all platforms?
Comment on attachment 273273 [details] Support the Internals setting on Mac View in context: https://bugs.webkit.org/attachment.cgi?id=273273&action=review >> Source/WebCore/testing/Internals.cpp:3500 >> +#if PLATFORM(IOS) || PLATFORM(MAC) > > why not support this for all platforms? Seems ViewportConfigutation is not supported on other platform. Got build failures in previous patches without this check.
Comment on attachment 273273 [details] Support the Internals setting on Mac Clearing flags on attachment: 273273 Committed r197766: <http://trac.webkit.org/changeset/197766>
All reviewed patches have been landed. Closing bug.
Why did this not get review from anyone who understands viewport scaling?
Re-opened since this is blocked by bug 155183
Comment on attachment 273273 [details] Support the Internals setting on Mac View in context: https://bugs.webkit.org/attachment.cgi?id=273273&action=review > Source/WebCore/testing/Internals.h:481 > + void setViewportForceAlwaysUserScalable(bool); > + double viewportConfigurationMaximumScale(); I don't think you need this stuff here if you use UIScriptController tests. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:220 > +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > +#include <WebCore/SoftLinking.h> > +SOFT_LINK_LIBRARY(libAccessibility) > +SOFT_LINK(libAccessibility, _AXSForceAllowWebScaling, Boolean, (), ()) > +SOFT_LINK_CONSTANT(libAccessibility, kAXSAllowForceWebScalingEnabledNotification, CFStringRef) > +#define kAXSAllowForceWebScalingEnabledNotification getkAXSAllowForceWebScalingEnabledNotification() > +#endif This is not a platform-specific file, and should not have this here. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:279 > +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > +static void forceAlwaysUserScalableChangedCallback(CFNotificationCenterRef, void* observer, CFStringRef, const void*, CFDictionaryRef) > +{ > + ASSERT(observer); > + static_cast<WebPage*>(observer)->updateForceAlwaysUserScalable(); > +} > +#endif Ditto. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:569 > +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > + CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), this, forceAlwaysUserScalableChangedCallback, kAXSAllowForceWebScalingEnabledNotification, 0, CFNotificationSuspensionBehaviorDeliverImmediately); > +#endif Ditto. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:604 > +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > + CFNotificationCenterRemoveObserver(CFNotificationCenterGetLocalCenter(), this, nullptr, nullptr); > +#endif Ditto. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2835 > +#if PLATFORM(IOS) > +void WebPage::updateForceAlwaysUserScalable() > +{ > + bool forceAlwaysUserScalableEnabled = m_forceAlwaysUserScalable; > +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > + forceAlwaysUserScalableEnabled |= _AXSForceAllowWebScaling(); > +#endif > + m_viewportConfiguration.setForceAlwaysUserScalable(forceAlwaysUserScalableEnabled); > +} > +#endif Move to WebPageIOS.mm > LayoutTests/ChangeLog:9 > + * accessibility/ios-simulator/force-user-scalable-expected.txt: Added. > + * accessibility/ios-simulator/force-user-scalable.html: Added. I would prefer that these tests use UIScriptController to test that the view is actually zoomable. See tests under LayoutTests/fast/viewport/ios/
Sorry, but I feel this patch did not have sufficient review or exposure from WebKit2 owners, and had significant problems, so I rolled it out.
Comment on attachment 273273 [details] Support the Internals setting on Mac View in context: https://bugs.webkit.org/attachment.cgi?id=273273&action=review >> LayoutTests/ChangeLog:9 >> + * accessibility/ios-simulator/force-user-scalable.html: Added. > > I would prefer that these tests use UIScriptController to test that the view is actually zoomable. See tests under LayoutTests/fast/viewport/ios/ I've taken some look into this. Seems we have to expose a function to set the ViewportConfiguration in WebPage class so that UIScriptController can call that. However the TestController only has access to a WKPageRef. I guess I have to expose another function in WKPage to cast call the WebPage function? Correct me if I'm wrong. This seems to make things even more complicated.
(In reply to comment #34) > Comment on attachment 273273 [details] > Support the Internals setting on Mac > > View in context: > https://bugs.webkit.org/attachment.cgi?id=273273&action=review > > >> LayoutTests/ChangeLog:9 > >> + * accessibility/ios-simulator/force-user-scalable.html: Added. > > > > I would prefer that these tests use UIScriptController to test that the view is actually zoomable. See tests under LayoutTests/fast/viewport/ios/ > > I've taken some look into this. Seems we have to expose a function to set > the ViewportConfiguration in WebPage class so that UIScriptController can > call that. However the TestController only has access to a WKPageRef. I > guess I have to expose another function in WKPage to cast call the WebPage > function? Correct me if I'm wrong. This seems to make things even more > complicated. If forceAlwaysUserScalable were a setting, you could set it via internals in a test. Then you'd have to synthesize a pinch-out to simulate zooming.
Comment on attachment 273273 [details] Support the Internals setting on Mac View in context: https://bugs.webkit.org/attachment.cgi?id=273273&action=review >>>> LayoutTests/ChangeLog:9 >>>> + * accessibility/ios-simulator/force-user-scalable.html: Added. >>> >>> I would prefer that these tests use UIScriptController to test that the view is actually zoomable. See tests under LayoutTests/fast/viewport/ios/ >> >> I've taken some look into this. Seems we have to expose a function to set the ViewportConfiguration in WebPage class so that UIScriptController can call that. However the TestController only has access to a WKPageRef. I guess I have to expose another function in WKPage to cast call the WebPage function? Correct me if I'm wrong. This seems to make things even more complicated. > > If forceAlwaysUserScalable were a setting, you could set it via internals in a test. > > Then you'd have to synthesize a pinch-out to simulate zooming. It's only a setting in ViewportConfiguration. So should I expose ViewportConfiguration in Internals and add that setting?
(In reply to comment #36) > It's only a setting in ViewportConfiguration. So should I expose > ViewportConfiguration in Internals and add that setting? The issue here is that Internals is in WebCore, but the ViewportConfiguration lives in WebKit2. I don't think adding a ViewportConfiguration to Internals and just looking at maximumScale is a good way to test, though; that's just testing a small bit of code in ViewportConfiguration, but you really need to test that the user experience works. Can you programmatically change the answer given by _AXSForceAllowWebScaling() on a per-process basis?
Created attachment 273454 [details] patch - Moved the SOFT_LINK code to WebPageIOS.mm - Added the test to make sure zooming is working correctly.
Created attachment 273458 [details] patch Fixed conflicts.
Comment on attachment 273458 [details] patch Attachment 273458 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/949774 New failing tests: accessibility/ios-simulator/force-always-user-scalable.html
Created attachment 273465 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 273458 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273458&action=review > Source/WebCore/page/ViewportConfiguration.h:93 > - double maximumScale() const { return m_configuration.maximumScale; } > + double maximumScale() const { return m_forceAlwaysUserScalable ? forceAlwaysUserScalableMaximumScale : m_configuration.maximumScale; } This solution doesn't prevent a page from saying minScale=10, maxScale=10 to avoid scalability. Also, the various default configurations have maxScale=5. Why did you choose 10? > Source/WebCore/testing/Internals.cpp:213 > +SOFT_LINK_LIBRARY(libAccessibility) > +SOFT_LINK(libAccessibility, _AXSSetForceAllowWebScaling, void, (Boolean scalable), (scalable)) Can we just hard link? > Source/WebCore/testing/Internals.cpp:3534 > +void Internals::setViewportForceAlwaysUserScalable(bool scalable) > +{ > +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > + _AXSSetForceAllowWebScaling(scalable); > +#else > + UNUSED_PARAM(scalable); > +#endif > +} You need to reset this at the end of every test that changes it. There's a mechanism to do that. > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:131 > + forceAlwaysUserScalable |= _AXSForceAllowWebScaling(); It's a shame that we have a WebPreference key or this, but you're bypassing that. Should we remove the key, or should the default value of that key be overridden by _AXSForceAllowWebScaling()? > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:140 > +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > + CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), this, forceAlwaysUserScalableChangedCallback, kAXSAllowForceWebScalingEnabledNotification, 0, CFNotificationSuspensionBehaviorDeliverImmediately); > +#endif This soft link of kAXSAllowForceWebScalingEnabledNotification is going to cause a performance hit for everyone on iOS. Can we just hard link? Does the CFNotificationCenterAddObserver have to happen here in WebPage? Lots of other notifications happen in WKWebView. Does it have to happen right at platformInitialize time?
Comment on attachment 273458 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273458&action=review >> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:131 >> + forceAlwaysUserScalable |= _AXSForceAllowWebScaling(); > > It's a shame that we have a WebPreference key or this, but you're bypassing that. Should we remove the key, or should the default value of that key be overridden by _AXSForceAllowWebScaling()? The current implementation will also respect the WebPreference key result. I think once all the accessibility setting code were in we can decide to remove it or not.
Created attachment 273622 [details] patch Review comments. - Hard linked the library - Moved notification code to WKWebView
Comment on attachment 273622 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273622&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:32265 > + "OTHER_LDFLAGS[sdk=iphoneos*]" = ( > + "$(ASAN_OTHER_LDFLAGS)", > + "-lAccessibility", > + ); > + "OTHER_LDFLAGS[sdk=iphonesimulator*]" = ( > + "$(ASAN_OTHER_LDFLAGS)", > + "-lAccessibility", This needs to be done via the .xcconfig files. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:32277 > + "$(ASAN_OTHER_LDFLAGS)", > + "-lAccessibility", > + ); Ditto. > Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:9604 > + "OTHER_LDFLAGS[sdk=iphoneos*]" = ( > + "$(OTHER_LDFLAGS)", > + "-l$(WEBKIT_SYSTEM_INTERFACE_LIBRARY)", > + "-lAccessibility", > + ); > + "OTHER_LDFLAGS[sdk=iphonesimulator*]" = ( > + "$(OTHER_LDFLAGS)", > + "-l$(WEBKIT_SYSTEM_INTERFACE_LIBRARY)", > + "-lAccessibility", Also needs to move to xcconfig file.
Created attachment 273641 [details] build fix patch Moved linker flags to xcconfig files.
Attachment 273641 [details] did not pass style-queue: ERROR: Source/WebCore/testing/Internals.cpp:215: _AXSSetForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 273641 [details] build fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=273641&action=review > Source/WebCore/testing/Internals.cpp:212 > +#include "AccessibilitySupport.h" This should be <AccessibilitySupport.h> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:106 > +#include "AccessibilitySupport.h" Ditto
Created attachment 273649 [details] patch review comments
Attachment 273649 [details] did not pass style-queue: ERROR: Source/WebCore/testing/Internals.cpp:215: _AXSSetForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 273649 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273649&action=review > LayoutTests/fast/viewport/ios/force-always-user-scalable.html:24 > + window.internals.setViewportForceAlwaysUserScalable(true); We should also try setting this back to NO and then check th scale is back to the author specified value
Created attachment 273657 [details] patch Updated test
Attachment 273657 [details] did not pass style-queue: ERROR: Source/WebCore/testing/Internals.cpp:215: _AXSSetForceAllowWebScaling is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 273657 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273657&action=review > Source/WebCore/Configurations/WebCoreTestSupport.xcconfig:53 > +OTHER_LDFLAGS[sdk=iphoneos*] = $(ASAN_OTHER_LDFLAGS) -lAccessibility; > +OTHER_LDFLAGS[sdk=iphonesimulator*] = $(ASAN_OTHER_LDFLAGS) -lAccessibility; Is it OK to link with Accessibility all the way back to Monarch? We have open source builders on Monarch. > Source/WebCore/page/ViewportConfiguration.cpp:194 > + if (m_forceAlwaysUserScalable) > + minimumScale = std::min(minimumScale, forceAlwaysUserScalableMinimumScale); This worries me a bit because there are pages which load by default with scale > 1 (e.g. "width=200px" in the viewport meta tag), and you're going to cause them to shrink down.
Comment on attachment 273657 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273657&action=review >> Source/WebCore/Configurations/WebCoreTestSupport.xcconfig:53 >> +OTHER_LDFLAGS[sdk=iphonesimulator*] = $(ASAN_OTHER_LDFLAGS) -lAccessibility; > > Is it OK to link with Accessibility all the way back to Monarch? We have open source builders on Monarch. should be ok. that dylib has been there since 3.0
Comment on attachment 273657 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=273657&action=review >> Source/WebCore/page/ViewportConfiguration.cpp:194 >> + minimumScale = std::min(minimumScale, forceAlwaysUserScalableMinimumScale); > > This worries me a bit because there are pages which load by default with scale > 1 (e.g. "width=200px" in the viewport meta tag), and you're going to cause them to shrink down. I've kept all the minimumScale calculation logic below the same. I suppose it would also respect the layout restrictions.
Comment on attachment 273657 [details] patch Clearing flags on attachment: 273657 Committed r197986: <http://trac.webkit.org/changeset/197986>
(In reply to comment #57) > Comment on attachment 273657 [details] > patch > > Clearing flags on attachment: 273657 > > Committed r197986: <http://trac.webkit.org/changeset/197986> Build fix for Production builds: Committed r197996: <http://trac.webkit.org/changeset/197996>