Bug 49452

Created attachment 73756 [details] patch

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

Committed revision 72052

(In reply to comment #5) > Committed revision 72052 And Qt specific expected results are updated in: http://trac.webkit.org/changeset/72070

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

There's another regression, bug 63367.

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

I also wonder if rolling this out is a possibility.

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