Bug 38160 - <input type="search"> with uneven padding causes text clipping
: <input type="search"> with uneven padding causes text clipping
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P2 Normal
Assigned To:
:
:
: 38253
:
  Show dependency treegraph
 
Reported: 2010-04-26 19:50 PST by
Modified: 2010-04-28 03:47 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-26 19:50:26 PST
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 From 2010-04-26 19:57:11 PST -------
Created an attachment (id=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 From 2010-04-26 19:58:25 PST -------
Created an attachment (id=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 From 2010-04-26 20:25:42 PST -------
(From update of attachment 54372 [details])
> +    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 From 2010-04-26 22:09:17 PST -------
> > +    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 From 2010-04-26 22:10:57 PST -------
Created an attachment (id=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 From 2010-04-26 22:50:11 PST -------
(From update of attachment 54381 [details])
> +        * 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 From 2010-04-27 09:58:00 PST -------
(In reply to comment #6)
> (From update of attachment 54381 [details] [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 From 2010-04-27 10:00:06 PST -------
Created an attachment (id=54423) [details]
[PATCH] Addressed Comments - Added ASSERTs

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