Bug 81802 – REGRESSION (r110065-r110080): fast/forms/placeholder-set-attribute.html is failing intermittently because WebKit fails to repaint after setting the placeholder attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

To reproduce, open fast/forms/placeholder-set-attribute.html in Safari. Reload a few times. Occasionally, the placeholder is missing. Forcing a repaint makes it appear in those cases. This is causing intermittent pixel failures.

[Radar WebKit Bug Importer]

<rdar://problem/11091412>

This is a regression. It doesn’t happen in Safari 5.1.4.

Added this test to the Mac skipped list in <http://trac.webkit.org/r111571>.

This regression occurred between r110065 and r110080. It was most likely caused by <http://trac.webkit.org/r110072>, the fix for bug 75568.

[Adele Peterson]

Julien, can you take a look at this?

[Julien Chaffraix]

(In reply to comment #5)
> Julien, can you take a look at this?

Sure, sorry I am not as responsive as I would like on those regressions. I should have some time next week to investigate this bug.

[Adele Peterson]

Thanks!!

[Julien Chaffraix]

Created attachment 133687 [details]
Proposed fix. Unfortunately no new test.

Comment on attachment 133687 [details]
Proposed fix. Unfortunately no new test.

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

r=me if you correct the style

> Source/WebCore/ChangeLog:11
> +        REGRESSION (r110065-r110080): fast/forms/placeholder-set-attribute.html is failing intermittently because WebKit fails to repaint after setting the placeholder attribute
> +        https://bugs.webkit.org/show_bug.cgi?id=81802
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Covered by fast/forms/placeholder-set-attribute.html which should be less flaky.
> +
> +        Unfortunately no new test case as this bug requires a very specific set of conditions that I couldn't reproduce deterministically.
> +

Can you explain how the change (presumably r110072) caused this regression?

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:297
> +        if (!placeholderBoxHadLayout && placeholderBox->checkForRepaintDuringLayout())
> +            // This assumes a shadow tree without floats. If floats are added, the
> +            // logic should be shared with RenderBlock::layoutBlockChild.
> +            placeholderBox->repaint();

This multi-line clause needs to be in braces.

[Julien Chaffraix]

Comment on attachment 133687 [details]
Proposed fix. Unfortunately no new test.

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

>> Source/WebCore/ChangeLog:11
>> +
> 
> Can you explain how the change (presumably r110072) caused this regression?

Sure, currently repainting goes through different means and one of them is RenderLayer. In this case, the placeholder has overflow: hidden set and was implicitly relying on the layer to properly repaint the first time. r110072 removed the layer, removing  also the first repaint. I will add this information in the ChangeLog.

[Julien Chaffraix]

Created attachment 133826 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 133826 [details]
Patch for landing

Clearing flags on attachment: 133826

Committed r112114: <http://trac.webkit.org/changeset/112114>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.