Bug 38160 - <input type="search"> with uneven padding causes text clipping
: <input type="search"> with uneven padding causes text clipping
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P2 Normal
Assigned To: Nobody
:
Depends on: 38253
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-26 19:50 PDT by Joseph Pecoraro
Modified: 2010-04-28 03:47 PDT (History)
2 users (show)

See Also:


Attachments
[PATCH] Position the clipRect where it should be (25.68 KB, patch)
2010-04-26 19:57 PDT, Joseph Pecoraro
mitz: review-
Details | Formatted Diff | Diff
[TEST] Test Case Showing Problem (211 bytes, text/html)
2010-04-26 19:58 PDT, Joseph Pecoraro
no flags Details
[PATCH] Addressed Comments - Fixed Caps Lock (27.17 KB, patch)
2010-04-26 22:10 PDT, Joseph Pecoraro
mitz: review-
Details | Formatted Diff | Diff
[PATCH] Addressed Comments - Added ASSERTs (27.21 KB, patch)
2010-04-27 10:00 PDT, Joseph Pecoraro
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2010-04-26 19:50:26 PDT
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
Comment 1 Joseph Pecoraro 2010-04-26 19:57:11 PDT
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
Comment 2 Joseph Pecoraro 2010-04-26 19:58:25 PDT
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 3 mitz@webkit.org 2010-04-26 20:25:42 PDT
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.
Comment 4 Joseph Pecoraro 2010-04-26 22:09:17 PDT
> > +    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.
Comment 5 Joseph Pecoraro 2010-04-26 22:10:57 PDT
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 6 mitz@webkit.org 2010-04-26 22:50:11 PDT
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().
Comment 7 Joseph Pecoraro 2010-04-27 09:58:00 PDT
(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!
Comment 8 Joseph Pecoraro 2010-04-27 10:00:06 PDT
Created attachment 54423 [details]
[PATCH] Addressed Comments - Added ASSERTs

Tested a bunch of pages with text / searches, as well as run-webkit-tests.
Comment 9 Joseph Pecoraro 2010-04-27 10:20:33 PDT
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