[iPadOS] REGRESSION(r247667): Autosizing style changes don't invalidate RenderText's preferred logical widths

[Myles C. Maxfield]

[iOS] [iPadOS] REGRESSION(r247667): Autosizing style changes don't invalidate RenderText's preferred logical widths

[Myles C. Maxfield]

Created attachment 391436 [details]
Needs test

[Myles C. Maxfield]

Created attachment 391444 [details]
Patch

[Myles C. Maxfield]

Created attachment 391445 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/59463898>

[Darin Adler]

Comment on attachment 391445 [details]
Patch

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

This looks like a good solution

> Source/WebCore/style/StyleAdjuster.h:55
> +        operator bool() const { return newFontSize || newLineHeight; }

Maybe "explicit operator bool" is better here? I think that’s usually what we use when we can check something as a bool, but wouldn’t want to convert it into a bool.

[Simon Fraser (smfr)]

Comment on attachment 391445 [details]
Patch

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

> Source/WebCore/page/Page.cpp:2990
> +                        renderer.setStyle(WTFMove(newStyle), StyleDifference::Equal);

Isn't StyleDifference::Equal a lie here? Don't you need the right diff to trigger layout?

[Myles C. Maxfield]

Comment on attachment 391445 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:2990
>> +                        renderer.setStyle(WTFMove(newStyle), StyleDifference::Equal);
> 
> Isn't StyleDifference::Equal a lie here? Don't you need the right diff to trigger layout?

It isn't a lie, because it's being passed as "minimumStyleDifference" and RenderElement::setStyle computes the style diff itself, and computes "diff = std::max(diff, minimalStyleDifference);". So StyleDifference::Equal here acts as a noop.

[Simon Fraser (smfr)]

OK

[zalan]

(In reply to Myles C. Maxfield from comment #7)
> Comment on attachment 391445 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391445&action=review
> 
> >> Source/WebCore/page/Page.cpp:2990
> >> +                        renderer.setStyle(WTFMove(newStyle), StyleDifference::Equal);
> > 
> > Isn't StyleDifference::Equal a lie here? Don't you need the right diff to trigger layout?
> 
> It isn't a lie, because it's being passed as "minimumStyleDifference" and
> RenderElement::setStyle computes the style diff itself, and computes "diff =
> std::max(diff, minimalStyleDifference);". So StyleDifference::Equal here
> acts as a noop.
I'd just rely on the default value then.

[Myles C. Maxfield]

> I'd just rely on the default value then.

Default value!!!! 😮

[Myles C. Maxfield]

Committed r257373: <https://trac.webkit.org/changeset/257373>

[Myles C. Maxfield]

https://bugs.webkit.org/show_bug.cgi?id=208231 fixes the test failure

[Darin Adler]

Comment on attachment 391445 [details]
Patch

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

> Source/WebCore/style/StyleAdjuster.cpp:637
> +bool Adjuster::adjustForTextAutosizing(RenderStyle& style, const Element& element, AdjustmentForTextAutosizing adjustment)
> +{
> +    AutosizeStatus::updateStatus(style);
> +    if (auto newFontSize = adjustment.newFontSize) {
> +        auto fontDescription = style.fontDescription();
> +        fontDescription.setComputedSize(*newFontSize);
> +        style.setFontDescription(WTFMove(fontDescription));
> +        style.fontCascade().update(&element.document().fontSelector());
> +    }
> +    if (auto newLineHeight = adjustment.newLineHeight)
> +        style.setLineHeight({ *newLineHeight, Fixed });
> +    return adjustment.newFontSize || adjustment.newLineHeight;
> +}

Should we also check if the size and height just *happen* to match what’s already in the style, and return bool if they aren’t changing?

[Ryan Haddad]

The test added with this change is a flaky failure on iOS. See https://bugs.webkit.org/show_bug.cgi?id=208245

[Ryan Haddad]

Since this broke an existing test (which EWS caught) and the newly added test is frequently failing, I think we should roll this out.

[Jacob Uphoff]

Reverted r257373 for reason:

This commit introduced one test that is a flaky failure on ios bots and broke another test

Committed r257481: <https://trac.webkit.org/changeset/257481>

[Myles C. Maxfield]

I fixed it already in https://bugs.webkit.org/show_bug.cgi?id=208231 but didn't land in time :(

[Myles C. Maxfield]

Created attachment 391770 [details]
Patch for rolling back in

[Myles C. Maxfield]

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

[Jacob Uphoff]

(In reply to Myles C. Maxfield from comment #17)
> I fixed it already in https://bugs.webkit.org/show_bug.cgi?id=208231 but
> didn't land in time :(

The fix you have in https://bugs.webkit.org/show_bug.cgi?id=208231 was for the test that broke with the commit that rolled out but there was no mention of fixing the test that was introduced that was flaky failing. Are you saying that was resolved in in 208231?

[Myles C. Maxfield]

The latest patch should deal with the flakes, and EWS is all green. I'm going to cq+ and watch the bots.

[WebKit Commit Bot]

Comment on attachment 391770 [details]
Patch for rolling back in

Clearing flags on attachment: 391770

Committed r257550: <https://trac.webkit.org/changeset/257550>

[Ryan Haddad]

(In reply to Myles C. Maxfield from comment #21)
> The latest patch should deal with the flakes, and EWS is all green. I'm
> going to cq+ and watch the bots.
fast/text-autosizing/ios/idempotentmode/viewport-change-relayout.html is failing on iOS bots again after re-landing the change:
https://build.webkit.org/results/Apple%20iOS%2013%20Simulator%20Release%20WK2%20(Tests)/r257568%20(2790)/fast/text-autosizing/ios/idempotentmode/viewport-change-relayout-diffs.html

I re-opened https://bugs.webkit.org/show_bug.cgi?id=208245

[Myles C. Maxfield]

Fixed in r257651.