Bug 38468 - REGRESSION: Text clipped in absolutely positioned search inputs
: REGRESSION: Text clipped in absolutely positioned search inputs
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P1 Normal
Assigned To: Nobody
: InRadar, Regression
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-03 08:40 PDT by Sidney San Martín
Modified: 2010-11-29 10:35 PST (History)
4 users (show)

See Also:


Attachments
Test case (352 bytes, text/html)
2010-05-03 08:40 PDT, Sidney San Martín
no flags Details
[PATCH] Quick Fix (1.60 KB, patch)
2010-05-03 16:53 PDT, Joseph Pecoraro
mitz: review-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix Based on My Comments (47.32 KB, patch)
2010-05-03 19:36 PDT, Joseph Pecoraro
mitz: review-
Details | Formatted Diff | Diff
[PATCH] Clip like text input (47.32 KB, patch)
2010-05-03 21:56 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 Sidney San Martín 2010-05-03 08:40:36 PDT
Created attachment 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 Alexey Proskuryakov 2010-05-03 13:47:58 PDT
Caused by <http://trac.webkit.org/changeset/58627>?
Comment 2 Alexey Proskuryakov 2010-05-03 15:20:53 PDT
<rdar://problem/7936843>
Comment 3 Joseph Pecoraro 2010-05-03 16:15:01 PDT
Ahh yes, this is related to my change. I'm discussing a fix now.
Comment 4 Joseph Pecoraro 2010-05-03 16:53:40 PDT
Created attachment 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 mitz@webkit.org 2010-05-03 17:06:47 PDT
Comment on attachment 54974 [details]
[PATCH] Quick Fix

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 mitz@webkit.org 2010-05-03 17:32:39 PDT
Comment on attachment 54974 [details]
[PATCH] Quick Fix

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 mitz@webkit.org 2010-05-03 17:45:04 PDT
(In reply to comment #6)
> Please just revert to using the content box. 

Sorry, I meant the inner block’s box.
Comment 8 Joseph Pecoraro 2010-05-03 17:51:48 PDT
> 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 mitz@webkit.org 2010-05-03 17:58:20 PDT
(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 Joseph Pecoraro 2010-05-03 19:11:42 PDT
> 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 Joseph Pecoraro 2010-05-03 19:16:48 PDT
The example code would have the clipRect.move(tx, ty) as well ;)
Comment 12 Joseph Pecoraro 2010-05-03 19:36:43 PDT
Created attachment 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 mitz@webkit.org 2010-05-03 20:39:02 PDT
Comment on attachment 54984 [details]
[PATCH] Proposed Fix Based on My Comments

> +    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 mitz@webkit.org 2010-05-03 20:40:50 PDT
(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 Joseph Pecoraro 2010-05-03 21:41:08 PDT
(In reply to comment #13)
> (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)?

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 Joseph Pecoraro 2010-05-03 21:56:33 PDT
Created attachment 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 mitz@webkit.org 2010-05-03 21:59:41 PDT
Comment on attachment 54994 [details]
[PATCH] Clip like text input

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 Joseph Pecoraro 2010-05-04 08:47:48 PDT
> 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 Joseph Pecoraro 2010-05-04 08:56:55 PDT
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 Noam Rosenthal 2010-11-25 07:47:27 PST
Comment on attachment 54994 [details]
[PATCH] Clip like text input

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