Bug 38468 - REGRESSION: Text clipped in absolutely positioned search inputs
: REGRESSION: Text clipped in absolutely positioned search inputs
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P1 Normal
Assigned To:
:
: InRadar, Regression
:
:
  Show dependency treegraph
 
Reported: 2010-05-03 08:40 PST by
Modified: 2010-11-29 10:35 PST (History)


Attachments
Test case (352 bytes, text/html)
2010-05-03 08:40 PST, Sidney San Martín
no flags Details
[PATCH] Quick Fix (1.60 KB, patch)
2010-05-03 16:53 PST, Joseph Pecoraro
mitz: review-
Review Patch | Details | Formatted Diff | Diff
[PATCH] Proposed Fix Based on My Comments (47.32 KB, patch)
2010-05-03 19:36 PST, Joseph Pecoraro
mitz: review-
Review Patch | Details | Formatted Diff | Diff
[PATCH] Clip like text input (47.32 KB, patch)
2010-05-03 21:56 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-05-03 08:40:36 PST
Created an attachment (id=54929) [details]
Test case

As an absolutely-positioned search input is positioned away from 0,0,  text inside it becomes clipped on the top and left sides. See attachment, hover over the input to change its position.

New in r58638 nightly.
------- Comment #1 From 2010-05-03 13:47:58 PST -------
Caused by <http://trac.webkit.org/changeset/58627>?
------- Comment #2 From 2010-05-03 15:20:53 PST -------
<rdar://problem/7936843>
------- Comment #3 From 2010-05-03 16:15:01 PST -------
Ahh yes, this is related to my change. I'm discussing a fix now.
------- Comment #4 From 2010-05-03 16:53:40 PST -------
Created an attachment (id=54974) [details]
[PATCH] Quick Fix

This is just a quick fix to the issue. This matches RenderButton's implementation. It is rect containing the contents (with padding but without borders).

Dan Bernstein also raised a good question:

  I think the clip rect is essential for when the field height is less than the
  text height. I just don’t know why it isn’t applied to non-search fields
  just as well.

This patch does not address those issues, only fixing my mistake.

I could add a patch to this test, but I think this was just fixing the blunder on my part. But, if a test is desired I could modify the attached test and add another pixel test.
------- Comment #5 From 2010-05-03 17:06:47 PST -------
(From update of attachment 54974 [details])
This requires a pixel text. I am not convinced that it’s correct to include padding in the clip rect. In other words, I think bug 38253 was invalid.
------- Comment #6 From 2010-05-03 17:32:39 PST -------
(From update of attachment 54974 [details])
Please just revert to using the content box. Looking at <http://trac.webkit.org/log/trunk/LayoutTests/platform/mac/fast/forms/search-zoomed-expected.png>, the search icon had always been clipped in that test.
------- Comment #7 From 2010-05-03 17:45:04 PST -------
(In reply to comment #6)
> Please just revert to using the content box. 

Sorry, I meant the inner block’s box.
------- Comment #8 From 2010-05-03 17:51:48 PST -------
> Please just revert to using the content box. Looking at
> <http://trac.webkit.org/log/trunk/LayoutTests/platform/mac/fast/forms/search-zoomed-expected.png>,
> the search icon had always been clipped in that test.

Correct, it was clipped before. I mentioned in bug 38253 that the new behavior (had it been correct) was now including a larger shadow that was being clipped before. I would think the desired behavior would be to not clip the SearchResultsIcon.


> This requires a pixel text. I am not convinced that it’s correct to include
> padding in the clip rect. In other words, I think bug 38253 was invalid.

For my education what are potential problems from including the padding in the clip rect? RenderButton includes the padding, could it have an issue?
------- Comment #9 From 2010-05-03 17:58:20 PST -------
(In reply to comment #8)
> For my education what are potential problems from including the padding in the
> clip rect?

It alters the appearance of this example
<input type="search" style="height: 25px; padding: 4px; -webkit-appearance: textfield; font-size: 20px;" value="Clipped">
from what it’s like in Safari 4.0.5. Seems like such behavioral changes are outside the scope of the original “uneven padding‘ bug you are fixing.
------- Comment #10 From 2010-05-03 19:11:42 PST -------
> It alters the appearance of this example

I see what you mean. However still, none of my patches handle this correctly.

> <input type="search" style="height: 25px; padding: 4px; -webkit-appearance: textfield; font-size: 20px;" value="Clipped">

Just using the centered innerBlock ends up with just the top half of the text. In this case the height of the innerBlock is 23px, while the height of the entire textfield is set at 25px. That means, to center the innerBlock it moves the clipRect just 1 pixel down.

In Safari 4.0.5 the clipRect was always offset by the borderTop()+paddingTop() ending up moving down 6px.

In both cases the clipRect's contentHeight is 13px (25 - 8 padding - 4 border) but you still see different portions.

I can make a patch with the best of both worlds, and end up with something like:

    RenderBox* renderBox = m_innerBlock->renderBox();
    IntRect clipRect = IntRect(renderBox->x(),
        max(borderTop() + paddingTop(), renderBox->y()),
        contentWidth(), contentHeight());

This way it looks like Safari 4.0.5 for cases where centering the clipRect would actually show less then it used to. This would still work for uneven padding cases.
------- Comment #11 From 2010-05-03 19:16:48 PST -------
The example code would have the clipRect.move(tx, ty) as well ;)
------- Comment #12 From 2010-05-03 19:36:43 PST -------
Created an attachment (id=54984) [details]
[PATCH] Proposed Fix Based on My Comments

Updated the original pixel test to include a second search box (to catch and x() y() blunders). This second search box is the example Dan Bernstein mentioned showing it does not change.
------- Comment #13 From 2010-05-03 20:39:02 PST -------
(From update of attachment 54984 [details])
> +    IntRect clipRect = IntRect(renderBox->x(), max(borderTop() + paddingTop(), renderBox->y()), contentWidth(), contentHeight());        

Wouldn’t this lead to asymmetry between the case of (top padding with no bottom padding) and (bottom padding with no top padding)?

Anyway, looking at this again, and comparing with the behavior of <input type="text">, which uses overflow: hidden on the text block in order to clip (this is why it doesn’t need to have control clip), I now think that the case I last mentioned is not worth preserving, and that one should just clip to the inner block, which would be consistent with text fields.
------- Comment #14 From 2010-05-03 20:40:50 PST -------
(In reply to comment #13)
> clip to the inner block

I mean something like

-    IntRect clipRect = IntRect(x(), y(), width(), height());        
+    IntRect clipRect = IntRect(m_innerBlock->renderBox()->frameRect());
------- Comment #15 From 2010-05-03 21:41:08 PST -------
(In reply to comment #13)
> (From update of attachment 54984 [details] [details])
> > +    IntRect clipRect = IntRect(renderBox->x(), max(borderTop() + paddingTop(), renderBox->y()), contentWidth(), contentHeight());        
> 
> Wouldn’t this lead to asymmetry between the case of (top padding with no bottom
> padding) and (bottom padding with no top padding)?

There would not be asymmetry because the "renderBox->y()" is a vertical center, and so whether there is more top or more bottom padding the y location would be set to the middle. That is unless the height of the innerBlock is already greater than the height of the textfield, where centering does not apply.


> Anyway, looking at this again, and comparing with the behavior of <input
> type="text">, which uses overflow: hidden on the text block in order to clip
> (this is why it doesn’t need to have control clip), I now think that the case I
> last mentioned is not worth preserving, and that one should just clip to the
> inner block, which would be consistent with text fields.

Good to know. Thanks!
------- Comment #16 From 2010-05-03 21:56:33 PST -------
Created an attachment (id=54994) [details]
[PATCH] Clip like text input

With the suggested solution. =)

If desired, I can add a text input to the test to compare the search field too.
------- Comment #17 From 2010-05-03 21:59:41 PST -------
(From update of attachment 54994 [details])
r=me

I can’t help commenting on something:

> +    ASSERT(m_innerBlock);
> +
> +    IntRect clipRect = IntRect(m_innerBlock->renderBox()->frameRect());

In my opinion, asserting that a pointer is not null is pointless if you are immediately going to dereference the pointer—you will crash anyway and you’ll be able to tell why. But maybe others think differently.
------- Comment #18 From 2010-05-04 08:47:48 PST -------
> In my opinion, asserting that a pointer is not null is pointless if you are
> immediately going to dereference the pointer—you will crash anyway and you’ll
> be able to tell why. But maybe others think differently.

I agree.
------- Comment #19 From 2010-05-04 08:56:55 PST -------
Committed r58762
    M    WebCore/ChangeLog
    M    WebCore/rendering/RenderTextControlSingleLine.cpp
    M    LayoutTests/platform/mac/fast/css/input-search-padding-expected.checksum
    M    LayoutTests/platform/mac/fast/css/input-search-padding-expected.png
    M    LayoutTests/platform/mac/fast/css/input-search-padding-expected.txt
    M    LayoutTests/fast/css/input-search-padding.html
    M    LayoutTests/ChangeLog
r58762 = b8bcd3c35ce14584349425fe47cddf4ad9349735 (refs/remotes/trunk)
http://trac.webkit.org/changeset/58762
------- Comment #20 From 2010-11-25 07:47:27 PST -------
(From update of attachment 54994 [details])
>--- a/WebCore/rendering/RenderTextControlSingleLine.cpp
>+++ b/WebCore/rendering/RenderTextControlSingleLine.cpp
>@@ -400,7 +400,9 @@ IntRect RenderTextControlSingleLine::controlClipRect(int tx, int ty) const
> {
>     // This should only get called for search inputs.
>     ASSERT(hasControlClip());
>-    IntRect clipRect = IntRect(x(), y(), width(), height());        
>+    ASSERT(m_innerBlock);
>+
>+    IntRect clipRect = IntRect(m_innerBlock->renderBox()->frameRect());
>     clipRect.move(tx, ty);
>     return clipRect;
> }

Can this be explained? We're seeing performance issues in pages with search input because of clipping to the whole frameRect..
------- Comment #21 From 2010-11-29 10:19:24 PST -------
> Can this be explained? We're seeing performance issues in pages with search input
> because of clipping to the whole frameRect..

That was mitz's suggestion in comment 13 and 14. The explanation for it was:

>     ... and that one should just clip to the inner block, which would be
>     consistent with text fields.

What kind of performance issues are you seeing. Can you give some sample numbers,
or maybe open a performance bug with an attached test case? That would be preferred,
since this bug is marked as resolved.