Bug 208084

Created attachment 391436 [details]
Needs test

Created attachment 391444 [details]
Patch

Created attachment 391445 [details]
Patch

<rdar://problem/59463898>

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.

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?

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.

OK

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

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

Default value!!!! 😮

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

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

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?

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

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

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>

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

Created attachment 391770 [details]
Patch for rolling back in

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

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

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

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

Clearing flags on attachment: 391770

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

(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

Fixed in r257651.