Bug 49452 – Placeholder should not be swapped in and out of the text control's inner text element

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Adele Peterson]

There's no need to swap the placeholder text in and out of the inner text element.  Instead, just paint the text.  This reduces complexity and makes it easier to make independent decisions about the placeholder text and the text control value.

[Adele Peterson]

Created attachment 73756 [details]
patch

[Adele Peterson]

Comment on attachment 73756 [details]
patch

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

> WebCore/ChangeLog:22
> +        (WebCore::RenderTextControlMultiLine::textBoxInsetRight): Added.

There is probably a better name for these new methods.  I wanted RenderTextControl::paintObject to be able to easily get the starting position of the text for search fields and other text controls.

[Darin Adler]

Comment on attachment 73756 [details]
patch

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

> WebCore/rendering/RenderTextControl.cpp:572
> +            return;

Is it OK that this does not fall through to RenderBlock::paintObject? Maybe we could put all this code in another function so return can be used without skipping the call to the base class.

> WebCore/rendering/RenderTextControl.cpp:576
> +            return;

Same question.

> WebCore/rendering/RenderTextControl.cpp:590
> +        unsigned length = placeholderText.length();
> +        const UChar* string = placeholderText.characters();

I’m not sure you need these local variables. They are only used once.

> WebCore/rendering/RenderTextControl.cpp:596
> +            int textX;
> +            int textY = ty + borderTop() + paddingTop() + textRenderer->paddingTop() + placeholderStyle->font().ascent();

Since we will need an IntPoint to use this, how about using an IntPoint from the start and use setY and setX?

> WebCore/rendering/RenderTextControlSingleLine.cpp:680
> +        if (node()->isHTMLElement()) {

Given that we cast to HTMLInputElement it seems strange that this existing code you did not modify checks isHTMLElement rather than hasTagName(inputTag).

[Adele Peterson]

Comment on attachment 73756 [details]
patch

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

Thanks!

>> WebCore/rendering/RenderTextControl.cpp:572
>> +            return;
> 
> Is it OK that this does not fall through to RenderBlock::paintObject? Maybe we could put all this code in another function so return can be used without skipping the call to the base class.

This one's ok.  No need to paint if visibility isn't VISIBLE.

>> WebCore/rendering/RenderTextControl.cpp:576
>> +            return;
> 
> Same question.

This one's probably not ok.  I'll rework this.

>> WebCore/rendering/RenderTextControl.cpp:590
>> +        const UChar* string = placeholderText.characters();
> 
> I’m not sure you need these local variables. They are only used once.

I'll fix this.

>> WebCore/rendering/RenderTextControl.cpp:596
>> +            int textY = ty + borderTop() + paddingTop() + textRenderer->paddingTop() + placeholderStyle->font().ascent();
> 
> Since we will need an IntPoint to use this, how about using an IntPoint from the start and use setY and setX?

Will do.

>> WebCore/rendering/RenderTextControlSingleLine.cpp:680
>> +        if (node()->isHTMLElement()) {
> 
> Given that we cast to HTMLInputElement it seems strange that this existing code you did not modify checks isHTMLElement rather than hasTagName(inputTag).

I agree! I will fix this.

[Adele Peterson]

Committed revision 72052

[Csaba Osztrogonác]

(In reply to comment #5)
> Committed revision 72052

And Qt specific expected results are updated in: http://trac.webkit.org/changeset/72070

[Daniel Bates]

This regressed placeholder text wrapping in <textarea>s. See bug #51290.

[Dimitri Glazkov (Google)]

There's another regression, bug 63367.

[Dimitri Glazkov (Google)]

We should consider re-implementing this as a shadow DOM element. This way, all of the layoutey things would just work magically.

[Dimitri Glazkov (Google)]

I also wonder if rolling this out is a possibility.

[Adele Peterson]

Reimplementing as a shadow DOM element is a good idea.  I think rolling this out would re-introduce worse problems than we have now.