Bug 38468 – REGRESSION: Text clipped in absolutely positioned search inputs

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Sidney San Martín]

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.

[Alexey Proskuryakov]

Caused by <http://trac.webkit.org/changeset/58627>?

[Alexey Proskuryakov]

<rdar://problem/7936843>

[Joseph Pecoraro]

Ahh yes, this is related to my change. I'm discussing a fix now.

[Joseph Pecoraro]

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

(In reply to comment #6)
> Please just revert to using the content box. 

Sorry, I meant the inner block’s box.

[Joseph Pecoraro]

> 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?

(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.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

The example code would have the clipRect.move(tx, ty) as well ;)

[Joseph Pecoraro]

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

(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());

[Joseph Pecoraro]

(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!

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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

[Noam Rosenthal]

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

[Joseph Pecoraro]

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