Bug 157771 – AX: iOS: when bringing focus to a text field we may zoom the page even if author wanted max scale = 1

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

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>

[chris fleizach]

Created attachment 279068 [details]
Patch

[chris fleizach]

Created attachment 279070 [details]
Patch

[WebKit Commit Bot]

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.

[chris fleizach]

Comment on attachment 279070 [details]
Patch

ToT changed on me

[chris fleizach]

Created attachment 279180 [details]
Patch

[WebKit Commit Bot]

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.

[Simon Fraser (smfr)]

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()

[chris fleizach]

Created attachment 279186 [details]
Patch

[WebKit Commit Bot]

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.

[Simon Fraser (smfr)]

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.

[chris fleizach]

Created attachment 279265 [details]
Patch

[WebKit Commit Bot]

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.

[Simon Fraser (smfr)]

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?

[chris fleizach]

(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

[chris fleizach]

(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...

[Simon Fraser (smfr)]

(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.

[chris fleizach]

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

[WebKit Commit Bot]

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.

[chris fleizach]

@simon: how does this look?

[Tim Horton]

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?

[chris fleizach]

Created attachment 279552 [details]
Patch

[WebKit Commit Bot]

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.

[chris fleizach]

(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?

[Tim Horton]

Comment on attachment 279552 [details]
Patch

This seems much less crazy.

[chris fleizach]

http://trac.webkit.org/changeset/201294