Disabling text autosizing should prevent text autosizing
Created attachment 376831 [details] Patch
<rdar://problem/48095374>
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
Created attachment 376895 [details] Patch
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.
*** Bug 200088 has been marked as a duplicate of this bug. ***
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.
macOS disables autosizing.
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.
(In reply to Alex Christensen from comment #9) > macOS also has ENABLE(TEXT_AUTOSIZING) off Not true: ENABLE_TEXT_AUTOSIZING = ENABLE_TEXT_AUTOSIZING;
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?
(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.
(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.
Created attachment 376921 [details] Patch
(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.
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 commit-queue@webkit.org... 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
http://trac.webkit.org/r248966
(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.