Bug 200955 – Disabling text autosizing should prevent text autosizing

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Disabling text autosizing should prevent text autosizing

[Alex Christensen]

Created attachment 376831 [details]
Patch

[Alex Christensen]

<rdar://problem/48095374>

[Alex Christensen]

Comment on attachment 376831 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376831&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TextSize.mm:39
> +    [webView configuration].preferences._textAutosizingEnabled = NO;

Simon added this SPI in http://trac.webkit.org/r200534

[Alex Christensen]

Created attachment 376895 [details]
Patch

[Myles C. Maxfield]

You’re intentionally breaking some functionality, I think your change log or at least a comment in the bug needs to explain why this is okay.

[Myles C. Maxfield]

*** Bug 200088 has been marked as a duplicate of this bug. ***

[Alex Christensen]

I don't think this is breaking any functionality.  It's just making disabling text autosizing actually disable text autosizing.  There is nobody that disables it right now except our tests (and they weren't actually disabling it), but the radar shows someone who wants to disable it.

[Myles C. Maxfield]

macOS disables autosizing.

[Alex Christensen]

macOS also has ENABLE(TEXT_AUTOSIZING) off, so the text autosizing seems to have no effect as evidenced by opening this iOS test in a browser before and after this change being the same.

[Simon Fraser (smfr)]

(In reply to Alex Christensen from comment #9)
> macOS also has ENABLE(TEXT_AUTOSIZING) off

Not true: ENABLE_TEXT_AUTOSIZING = ENABLE_TEXT_AUTOSIZING;

[Myles C. Maxfield]

Comment on attachment 376895 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376895&action=review

> Source/WebCore/css/StyleResolver.cpp:1890
>  

I don't see anything in the rest of this function that appears to interact with the text zoom setting. Why does this patch work?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TextSize.mm:39
> +    [webView configuration].preferences._textAutosizingEnabled = NO;

Are both this line and "-webkit-text-size-adjust:none;" below needed? Why?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TextSize.mm:49
> +    EXPECT_WK_STREQ([webView stringByEvaluatingJavaScript:@"document.getElementById('testspan').offsetWidth"], "242");

Can we use Ahem so this is less flakey?

[Myles C. Maxfield]

(In reply to Simon Fraser (smfr) from comment #10)
> (In reply to Alex Christensen from comment #9)
> > macOS also has ENABLE(TEXT_AUTOSIZING) off
> 
> Not true: ENABLE_TEXT_AUTOSIZING = ENABLE_TEXT_AUTOSIZING;

Ah, yes, I was thinking of shipping iPad WebKit, not macOS WebKit. Shipping iPad enables ENABLE(TEXT_AUTOSIZING) but disables settings().textAutosizingEnabled(). Your patch would have broken that.

However, for unrelated reasons, we now have enabled settings().textAutosizingEnabled() on the current version of WebKit on iPad. You got lucky.

[Alex Christensen]

(In reply to Myles C. Maxfield from comment #11)
> Comment on attachment 376895 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376895&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:1890
> >  
> 
> I don't see anything in the rest of this function that appears to interact
> with the text zoom setting. Why does this patch work?
Without this patch, the API call that called setTextZoomFactor was setting the font size to 32, then the call to setComputedSize in StyleResolver::checkForTextSizeAdjust was setting the size back to 16 before laying out and drawing.  There were other settings checks, and this seemed to be a missing setting check that, when added, made it behave as desired.
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextSize.mm:39
> > +    [webView configuration].preferences._textAutosizingEnabled = NO;
> 
> Are both this line and "-webkit-text-size-adjust:none;" below needed? Why?
Yes.  The code path in !style.textSizeAdjust().isNone() seems to coincidentally do the right thing.
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextSize.mm:49
> > +    EXPECT_WK_STREQ([webView stringByEvaluatingJavaScript:@"document.getElementById('testspan').offsetWidth"], "242");
> 
> Can we use Ahem so this is less flakey?
Sure.

[Alex Christensen]

Created attachment 376921 [details]
Patch

[Myles C. Maxfield]

(In reply to Alex Christensen from comment #13)
> (In reply to Myles C. Maxfield from comment #11)
> > Comment on attachment 376895 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=376895&action=review
> > 
> > > Source/WebCore/css/StyleResolver.cpp:1890
> > >  
> > 
> > I don't see anything in the rest of this function that appears to interact
> > with the text zoom setting. Why does this patch work?
> Without this patch, the API call that called setTextZoomFactor was setting
> the font size to 32, then the call to setComputedSize in
> StyleResolver::checkForTextSizeAdjust was setting the size back to 16 before
> laying out and drawing.  There were other settings checks, and this seemed
> to be a missing setting check that, when added, made it behave as desired.
> > 
> > > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextSize.mm:39
> > > +    [webView configuration].preferences._textAutosizingEnabled = NO;
> > 
> > Are both this line and "-webkit-text-size-adjust:none;" below needed? Why?
> Yes.  The code path in !style.textSizeAdjust().isNone() seems to
> coincidentally do the right thing.

If this is true, can we just tell clients to stop setting -webkit-text-size-adjust: none? Would that fix the content?

> > 
> > > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextSize.mm:49
> > > +    EXPECT_WK_STREQ([webView stringByEvaluatingJavaScript:@"document.getElementById('testspan').offsetWidth"], "242");
> > 
> > Can we use Ahem so this is less flakey?
> Sure.

[WebKit Commit Bot]

Comment on attachment 376921 [details]
Patch

Rejecting attachment 376921 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 376921, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=376921&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=200955&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 376921 from bug 200955.
Fetching: https://bugs.webkit.org/attachment.cgi?id=376921
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	Tools/TestWebKitAPI/Tests/WebKitCocoa/TextSize.mm
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/text-autosizing/ios/text-size-adjust-inline-style.html
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/css/StyleResolver.cpp
	M	Tools/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/Tools/ChangeLog' is out of date
W: 4fa0e7a048df9ec6273a7d34b9c0fff8455076ed and refs/remotes/origin/master differ, using rebase:
:040000 040000 5796ec1750ea871fc69abfe9e61c8350c0327ac0 069f4b367228f6078c49377670524c5c1bb71133 M	LayoutTests
:040000 040000 e39ecd22acb5fc25e0edb598d8aadc66e0e819f1 81633bf5327f3eeb9504dd802a4883d578c6d7e7 M	Source
:040000 040000 e6e3faac5cb0721bea76dd69566084b01bf24abf 352ce974a62bcd29b698d80be7ffb43ec0ce22cc M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	Tools/TestWebKitAPI/Tests/WebKitCocoa/TextSize.mm
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/text-autosizing/ios/text-size-adjust-inline-style.html
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/css/StyleResolver.cpp
	M	Tools/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/Tools/ChangeLog' is out of date
W: 4fa0e7a048df9ec6273a7d34b9c0fff8455076ed and refs/remotes/origin/master differ, using rebase:
:040000 040000 5796ec1750ea871fc69abfe9e61c8350c0327ac0 069f4b367228f6078c49377670524c5c1bb71133 M	LayoutTests
:040000 040000 e39ecd22acb5fc25e0edb598d8aadc66e0e819f1 81633bf5327f3eeb9504dd802a4883d578c6d7e7 M	Source
:040000 040000 e6e3faac5cb0721bea76dd69566084b01bf24abf 352ce974a62bcd29b698d80be7ffb43ec0ce22cc M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From https://git.webkit.org/git/WebKit
   bad1df1c634..f84b4773cd3  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 248963 = bad1df1c63406694a228918c9431394201a38a74
r248964 = f84b4773cd38ad5d51df103f96e43c6fffe5ea86
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: https://webkit-queues.webkit.org/results/12953373

[Alex Christensen]

http://trac.webkit.org/r248966

[Alex Christensen]

(In reply to Myles C. Maxfield from comment #15)
> If this is true, can we just tell clients to stop setting
> -webkit-text-size-adjust: none? Would that fix the content?
I don't think so, but I don't think I understand why.  I think they need -webkit-text-size-adjust, _textAutosizingEnabled=NO, and _setTextZoomFactor, which is the combination that this change fixed.