[iOS] Implement idempotent mode for text autosizing
Created attachment 368190 [details] WIP
Created attachment 368202 [details] Patch
Comment on attachment 368202 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368202&action=review > Source/WebCore/rendering/RenderElement.h:201 > + static bool idempotentMode(); Is this used?
Comment on attachment 368202 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368202&action=review >> Source/WebCore/rendering/RenderElement.h:201 >> + static bool idempotentMode(); > > Is this used? Good catch! Nope.
<rdar://problem/50211034>
Created attachment 368271 [details] Patch
Created attachment 368273 [details] Patch
Comment on attachment 368273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368273&action=review > Source/WebCore/rendering/RenderBlockFlow.cpp:3738 > + pageScale = std::min(std::max(pageScale, 0.5f), 1.0f); > + for (auto& point : points) { > + float fraction = 3.0f - 3.0f * pageScale; > + point.setY(point.x() + (point.y() - point.x()) * fraction); > + } You do this work on every request to adjust text size, and then you skip points[i].x() < specifiedSize, so you only really need to scale two points, right? > Source/WebCore/rendering/RenderElement.cpp:2127 > + if (SettingsBase::textAutosizingUsesIdempotentMode() && style.maxHeight().type() == Fixed && is<RenderBlock>(renderer) && style.maxHeight().value() <= downcast<RenderBlock>(renderer).layoutOverflowRect().maxY()) > + return RenderObject::FixedHeight; Is this logic covered in your test(s)? > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3033 > + double initialScale = m_viewportConfiguration.initialScale(); > + m_page->setInitialScale(initialScale); Kinda stupid that m_viewportConfiguration isn't on Page in WebCore.
Created attachment 368281 [details] Patch
Created attachment 368283 [details] Patch
Comment on attachment 368283 [details] Patch Attachment 368283 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12000513 New failing tests: fast/text-autosizing/ios/idempotent-autosizing-identity.html fast/text-autosizing/ios/idempotent-autosizing.html
Created attachment 368296 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 368283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368283&action=review > LayoutTests/fast/text-autosizing/ios/idempotent-autosizing-identity.html:11 > +<script src="../../../../resources/js-test-pre.js"></script> three .. instead of four ..?
(In reply to Simon Fraser (smfr) from comment #8) > Comment on attachment 368273 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368273&action=review > > > Source/WebCore/rendering/RenderBlockFlow.cpp:3738 > > + pageScale = std::min(std::max(pageScale, 0.5f), 1.0f); > > + for (auto& point : points) { > > + float fraction = 3.0f - 3.0f * pageScale; > > + point.setY(point.x() + (point.y() - point.x()) * fraction); > > + } > > You do this work on every request to adjust text size, and then you skip > points[i].x() < specifiedSize, so you only really need to scale two points, > right? > > > Source/WebCore/rendering/RenderElement.cpp:2127 > > + if (SettingsBase::textAutosizingUsesIdempotentMode() && style.maxHeight().type() == Fixed && is<RenderBlock>(renderer) && style.maxHeight().value() <= downcast<RenderBlock>(renderer).layoutOverflowRect().maxY()) > > + return RenderObject::FixedHeight; > > Is this logic covered in your test(s)? > > > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3033 > > + double initialScale = m_viewportConfiguration.initialScale(); > > + m_page->setInitialScale(initialScale); > > Kinda stupid that m_viewportConfiguration isn't on Page in WebCore. Can we address these issues in a follow up?
(In reply to Jon Lee from comment #14) > (In reply to Simon Fraser (smfr) from comment #8) > > Comment on attachment 368273 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=368273&action=review > > > > > Source/WebCore/rendering/RenderBlockFlow.cpp:3738 > > > + pageScale = std::min(std::max(pageScale, 0.5f), 1.0f); > > > + for (auto& point : points) { > > > + float fraction = 3.0f - 3.0f * pageScale; > > > + point.setY(point.x() + (point.y() - point.x()) * fraction); > > > + } > > > > You do this work on every request to adjust text size, and then you skip > > points[i].x() < specifiedSize, so you only really need to scale two points, > > right? > > > > > Source/WebCore/rendering/RenderElement.cpp:2127 > > > + if (SettingsBase::textAutosizingUsesIdempotentMode() && style.maxHeight().type() == Fixed && is<RenderBlock>(renderer) && style.maxHeight().value() <= downcast<RenderBlock>(renderer).layoutOverflowRect().maxY()) > > > + return RenderObject::FixedHeight; > > > > Is this logic covered in your test(s)? > > > > > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3033 > > > + double initialScale = m_viewportConfiguration.initialScale(); > > > + m_page->setInitialScale(initialScale); > > > > Kinda stupid that m_viewportConfiguration isn't on Page in WebCore. > > Can we address these issues in a follow up? I won't speak for the others, but the last one is definitely not something to fix in this patch :P
Comment on attachment 368273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368273&action=review >> Source/WebCore/rendering/RenderElement.cpp:2127 >> + return RenderObject::FixedHeight; > > Is this logic covered in your test(s)? Nope. I don't think we want to test these details yet, as they are likely to change soon.
Created attachment 368302 [details] Patch for committing
Created attachment 368303 [details] Patch for committing
Committed r244682: <https://trac.webkit.org/changeset/244682>