Bug 38160 – <input type="search"> with uneven padding causes text clipping

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

[Joseph Pecoraro]

This occurs when the RenderTheme does not explicitly force the padding.

Also, this may be moot if a decision is made to change input padding behavior:
https://bugs.webkit.org/show_bug.cgi?id=32981

[Joseph Pecoraro]

Created attachment 54372 [details]
[PATCH] Position the clipRect where it should be

The innerBlock's location is being correctly set vertically here:
http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderTextControlSingleLine.cpp#L234

[Joseph Pecoraro]

Created attachment 54374 [details]
[TEST] Test Case Showing Problem

This test case shows the problem. Note to avoid the RenderTheme* path I use -webkit-appearance: none.

I also forgot to mention, the pixel difference I got on Mac was 0.22%.

Comment on attachment 54372 [details]
[PATCH] Position the clipRect where it should be

> +    RenderBox* renderer = static_cast<RenderBox*>(m_innerBlock->renderer());

0. You should use m_innerBlock->renderBox() instead of the unsafe cast.

I noticed some problems while reviewing this patch:

1. RenderTextControl’s override of hasControlClip() is redundant since it returns false just like the base class.
2. RenderTextControl’s implementation of controlClipRect() is used only by RenderTextControlSingleLine instances.
3. RenderTextControlSingleLine::paint() uses contentBoxRect() to paint the caps lock indicator and is therefore similarly broken with uneven padding (the indicator renders at the top instead of at the middle).
4. I don’t really understand the logic of RenderTextControlSingleLine::hasControlClip(): it allows things like
      <input value="overflow" style="height: 20px; font-size: 24px;">
   which seem quite wrong.

r- since I suspect you’ll want to address item 0 and perhaps some of the others.

[Joseph Pecoraro]

> > +    RenderBox* renderer = static_cast<RenderBox*>(m_innerBlock->renderer());
> 
> 0. You should use m_innerBlock->renderBox() instead of the unsafe cast.

Fixed.


> 1. RenderTextControl’s override of hasControlClip() is redundant since it
> returns false just like the base class.

Fixed. Good Catch.


> 2. RenderTextControl’s implementation of controlClipRect() is used only by
> RenderTextControlSingleLine instances.

Removed.


> 3. RenderTextControlSingleLine::paint() uses contentBoxRect() to paint the caps
> lock indicator and is therefore similarly broken with uneven padding (the
> indicator renders at the top instead of at the middle).

Fixed. Great Catch!


> 4. I don’t really understand the logic of
> RenderTextControlSingleLine::hasControlClip(): it allows things like
>       <input value="overflow" style="height: 20px; font-size: 24px;">
>    which seem quite wrong.

Could you explain? hasControlClip is something like "return m_cancelButton" which only happens if the element is a search field (or maybe has a -webkit-appearance value). I don't think the case you mentioned would be affected.

[Joseph Pecoraro]

Created attachment 54381 [details]
[PATCH] Addressed Comments - Fixed Caps Lock

I did a brief search and didn't see an easy way to test caps lock. I could investigate if that is really desired. Running all tests didn't produce any noticable failures. I'll be running pixel tests overnight.

Comment on attachment 54381 [details]
[PATCH] Addressed Comments - Fixed Caps Lock

> +        * rendering/RenderTextControl.cpp: moved controlClipRect implementation to TextControlSingleList

s/TextControlSingleList/RenderTextControlSingleLine/


> +        // Center vertically like the text.
> +        contentsRect.setY((height() - contentsRect.height()) / 2);
> +

Have you tested this with both even and odd heights?

> +IntRect RenderTextControlSingleLine::controlClipRect(int tx, int ty) const
> +{
> +    IntRect clipRect;
> +    if (!m_innerBlock)
> +        clipRect = contentBoxRect();

Does this case ever get hit? Is it covered by the test? This looks just as wrong as the present bug.

> +    virtual IntRect controlClipRect(int tx, int ty) const;

This should be private.

Sorry to r- again, but I don’t understand the !m_innerBlock branch of controlClipRect().

[Joseph Pecoraro]

(In reply to comment #6)
> (From update of attachment 54381 [details])
> > +        * rendering/RenderTextControl.cpp: moved controlClipRect implementation to TextControlSingleList
> 
> s/TextControlSingleList/RenderTextControlSingleLine/

eek. That was bad.


> > +        // Center vertically like the text.
> > +        contentsRect.setY((height() - contentsRect.height()) / 2);
> 
> Have you tested this with both even and odd heights?

Yep, I tested Caps Lock with odd, even, weird padding and no padding.


> > +IntRect RenderTextControlSingleLine::controlClipRect(int tx, int ty) const
> > +{
> > +    IntRect clipRect;
> > +    if (!m_innerBlock)
> > +        clipRect = contentBoxRect();
> 
> Does this case ever get hit? Is it covered by the test? This looks just as
> wrong as the present bug.

You're correct this case doesn't get hit. controlClipRect should only get called if hasClipRect returns true. The only direct call is in RenderBox. I've placed ASSERTs here.


> > +    virtual IntRect controlClipRect(int tx, int ty) const;
> 
> This should be private.

Done.


> Sorry to r- again, but I don’t understand the !m_innerBlock branch of
> controlClipRect().

Don't be sorry, I appreciate good reviews like this because I always learn something! Thanks!

[Joseph Pecoraro]

Created attachment 54423 [details]
[PATCH] Addressed Comments - Added ASSERTs

Tested a bunch of pages with text / searches, as well as run-webkit-tests.

[Joseph Pecoraro]

Committed r58313
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderTextControlSingleLine.h
	M	WebCore/rendering/RenderTextControl.cpp
	M	WebCore/rendering/RenderTextControlSingleLine.cpp
	M	WebCore/rendering/RenderTextControl.h
	A	LayoutTests/platform/mac/fast/css/input-search-padding-expected.checksum
	A	LayoutTests/platform/mac/fast/css/input-search-padding-expected.png
	A	LayoutTests/platform/mac/fast/css/input-search-padding-expected.txt
	A	LayoutTests/fast/css/input-search-padding.html
	M	LayoutTests/ChangeLog
r58313 = 61acf99ce5cc480259780ffd67fdfdaa9bdb4998 (trunk)
http://trac.webkit.org/changeset/58313