Summary: | AX: iOS: when bringing focus to a text field we may zoom the page even if author wanted max scale = 1 | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | commit-queue, enrica, jonlee, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | Other | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
chris fleizach
2016-05-16 16:48:34 PDT
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.
|