Bug 49452 - Placeholder should not be swapped in and out of the text control's inner text element
Summary: Placeholder should not be swapped in and out of the text control's inner text...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Adele Peterson
URL:
Keywords:
Depends on:
Blocks: 54814 63367 63927
  Show dependency treegraph
 
Reported: 2010-11-12 10:22 PST by Adele Peterson
Modified: 2011-07-05 02:25 PDT (History)
4 users (show)

See Also:


Attachments
patch (63.11 KB, patch)
2010-11-12 10:29 PST, Adele Peterson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adele Peterson 2010-11-12 10:22:36 PST
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.
Comment 1 Adele Peterson 2010-11-12 10:29:09 PST
Created attachment 73756 [details]
patch
Comment 2 Adele Peterson 2010-11-12 10:31:26 PST
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.
Comment 3 Darin Adler 2010-11-15 11:32:24 PST
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).
Comment 4 Adele Peterson 2010-11-15 11:55:26 PST
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.
Comment 5 Adele Peterson 2010-11-15 18:10:09 PST
Committed revision 72052
Comment 6 Csaba Osztrogonác 2010-11-16 03:11:11 PST
(In reply to comment #5)
> Committed revision 72052

And Qt specific expected results are updated in: http://trac.webkit.org/changeset/72070
Comment 7 Daniel Bates 2010-12-18 23:16:26 PST
This regressed placeholder text wrapping in <textarea>s. See bug #51290.
Comment 8 Dimitri Glazkov (Google) 2011-06-25 09:50:31 PDT
There's another regression, bug 63367.
Comment 9 Dimitri Glazkov (Google) 2011-06-25 10:03:23 PDT
We should consider re-implementing this as a shadow DOM element. This way, all of the layoutey things would just work magically.
Comment 10 Dimitri Glazkov (Google) 2011-06-25 10:05:46 PDT
I also wonder if rolling this out is a possibility.
Comment 11 Adele Peterson 2011-06-26 10:56:47 PDT
Reimplementing as a shadow DOM element is a good idea.  I think rolling this out would re-introduce worse problems than we have now.