If a user taps on a text field to bring keyboard focus, we should not zoom the page in if the author had set scaling to NO We should only do scaling if the user does it themselves <rdar://problem/25443355>
Created attachment 279068 [details] Patch
Created attachment 279070 [details] Patch
Attachment 279070 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1053: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1054: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 279070 [details] Patch ToT changed on me
Created attachment 279180 [details] Patch
Attachment 279180 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1053: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1054: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 279180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279180&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.h:512 > + // maximumPageScaleFactor accounts for when user scaling is enabled by force. > double maximumPageScaleFactor() const; > + > + // originalMaximumScaleFactor is the default page scaling of the page. > + double originalMaximumScaleFactor() const; I don't think these names are clear. In particular, "by force" is unclear. Maybe: maximumAllowablePageScaleFactor() maximumPageScaleFactorFromViewportTag()
Created attachment 279186 [details] Patch
Attachment 279186 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1053: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1054: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 279186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279186&action=review > Source/WebCore/page/ViewportConfiguration.h:89 > + bool forceAlwaysUserScalable() { return m_forceAlwaysUserScalable; } This should be const > Source/WebKit2/Shared/AssistedNodeInformation.h:103 > + bool userScalingForced { false }; This is also ambiguous. Does it mean that the user has zoomed when forceAlwaysUserScalable() is true, or does it just reflect forceAlwaysUserScalable()? I can't tell from this patch.
Created attachment 279265 [details] Patch
Attachment 279265 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1053: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1054: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 279265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279265&action=review > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1054 > + allowScaling:(_assistedNodeInformation.allowsUserScaling && !_assistedNodeInformation.forceAlwaysUserScalable && !UICurrentUserInterfaceIdiomIsPad()) Doesn't this mean that a user with "always allow scaling" set is going to never get zooming on an input on iPhone?
(In reply to comment #13) > Comment on attachment 279265 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=279265&action=review > > > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1054 > > + allowScaling:(_assistedNodeInformation.allowsUserScaling && !_assistedNodeInformation.forceAlwaysUserScalable && !UICurrentUserInterfaceIdiomIsPad()) > > Doesn't this mean that a user with "always allow scaling" set is going to > never get zooming on an input on iPhone? Possibly... Looking
(In reply to comment #14) > (In reply to comment #13) > > Comment on attachment 279265 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=279265&action=review > > > > > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1054 > > > + allowScaling:(_assistedNodeInformation.allowsUserScaling && !_assistedNodeInformation.forceAlwaysUserScalable && !UICurrentUserInterfaceIdiomIsPad()) > > > > Doesn't this mean that a user with "always allow scaling" set is going to > > never get zooming on an input on iPhone? > > Possibly... Looking Probably don't need _assistedNodeInformation.forceAlwaysUserScalable at all, since maximumPageScaleFactorFromViewportTag will be 1x in this case Rebuilding and testing...
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > Comment on attachment 279265 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=279265&action=review > > > > > > > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1054 > > > > + allowScaling:(_assistedNodeInformation.allowsUserScaling && !_assistedNodeInformation.forceAlwaysUserScalable && !UICurrentUserInterfaceIdiomIsPad()) > > > > > > Doesn't this mean that a user with "always allow scaling" set is going to > > > never get zooming on an input on iPhone? > > > > Possibly... Looking > > Probably don't need _assistedNodeInformation.forceAlwaysUserScalable at > all, since maximumPageScaleFactorFromViewportTag will be 1x in this case > > Rebuilding and testing... Please add a layout test for that case too.
Created attachment 279397 [details] Patch Added a unit test for the case where we don't want to zoom in and the case where we DO want to zoom in
Attachment 279397 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1054: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
@simon: how does this look?
Comment on attachment 279397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279397&action=review > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1047 > + // In case user scaling is force enabled, do not use that scaling when zomming in with an input field. sp: zomming > Source/WebKit2/WebProcess/WebPage/WebPage.h:509 > + // maximumAllowablePageScaleFactor is the max page scaling that also accounts for whether the preference for "always scale" is enabled. s/scaling/scale/ > Source/WebKit2/WebProcess/WebPage/WebPage.h:511 > + // maximumPageScaleFactorFromViewportTag is the default page scaling of the page. It does not account for when the preference for "always scale" is enabled. ditto. > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:340 > + return m_page->pageScaleFactor(); How is the current live page scale a "maximum" of any sort?
Created attachment 279552 [details] Patch
Attachment 279552 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1055: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #20) > Comment on attachment 279397 [details] > Patch > Thanks for review. Addressed comments and also removed unnecessary changes. The patch had morphed a bit from the beginning and we don't need nearly as many changes as originally thought thanks > View in context: > https://bugs.webkit.org/attachment.cgi?id=279397&action=review > > > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1047 > > + // In case user scaling is force enabled, do not use that scaling when zomming in with an input field. > > sp: zomming > > > Source/WebKit2/WebProcess/WebPage/WebPage.h:509 > > + // maximumAllowablePageScaleFactor is the max page scaling that also accounts for whether the preference for "always scale" is enabled. > > s/scaling/scale/ > > > Source/WebKit2/WebProcess/WebPage/WebPage.h:511 > > + // maximumPageScaleFactorFromViewportTag is the default page scaling of the page. It does not account for when the preference for "always scale" is enabled. > > ditto. > > > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:340 > > + return m_page->pageScaleFactor(); > > How is the current live page scale a "maximum" of any sort?
Comment on attachment 279552 [details] Patch This seems much less crazy.
http://trac.webkit.org/changeset/201294