Bug 197250 – [iOS] Implement idempotent mode for text autosizing

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

[Myles C. Maxfield]

[iOS] Implement idempotent mode for text autosizing

[Myles C. Maxfield]

Created attachment 368190 [details]
WIP

[Myles C. Maxfield]

Created attachment 368202 [details]
Patch

[Simon Fraser (smfr)]

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?

[Myles C. Maxfield]

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.

[Radar WebKit Bug Importer]

<rdar://problem/50211034>

[Myles C. Maxfield]

Created attachment 368271 [details]
Patch

[Myles C. Maxfield]

Created attachment 368273 [details]
Patch

[Simon Fraser (smfr)]

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.

[Myles C. Maxfield]

Created attachment 368281 [details]
Patch

[Myles C. Maxfield]

Created attachment 368283 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Jon Lee]

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

[Jon Lee]

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

[Tim Horton]

(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

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Created attachment 368302 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 368303 [details]
Patch for committing

[Myles C. Maxfield]

Committed r244682: <https://trac.webkit.org/changeset/244682>