Bug 80531 – REGRESSION(r110072): fast/forms/textfield-overflow.html is failing

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

The caret seems to be protruding out of the box which is not expected:


http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Fforms%2Ftextfield-overflow.html

I will investigate why.

[Julien Chaffraix]

Created attachment 131387 [details]
WIP fix, need to think about it / some feedbacks.

[Julien Chaffraix]

Adding people who have touched this code and could have good comments on the approach :-}

[Kent Tamura]

Comment on attachment 131387 [details]
WIP fix, need to think about it / some feedbacks.

View in context: https://bugs.webkit.org/attachment.cgi?id=131387&action=review

> LayoutTests/ChangeLog:14
> +        * platform/chromium-linux/fast/forms/textfield-overflow-expected.png:
> +        Not sure what to think about this one either. The caret is now touching the top
> +        edge but at the same time is going closer to the bottom one.

Won't this change update the render tree dump?

[Julien Chaffraix]

Comment on attachment 131387 [details]
WIP fix, need to think about it / some feedbacks.

View in context: https://bugs.webkit.org/attachment.cgi?id=131387&action=review

>> LayoutTests/ChangeLog:14
>> +        edge but at the same time is going closer to the bottom one.
> 
> Won't this change update the render tree dump?

Yes but that's tangential: 
* this WIP patch doesn't pretend to rebaseline all the tests that will change (for example some <input> elements will now get a layer).
* the tree dump was already changed by r110072 which removed the layer and that caused the care to move upwards.

[Kent Tamura]

Well, before r110072, the height of the shadow DIV (innerTextElement() in RenderTextControlSingleLine) was 4px because the height of <input> is 10px and borders and paddings were subtracted from 10px.

Since r110072, the height of the shadow DIV is 13px.  It should be 4px and I don't know why r110072 changed this.
I'm interested in the height with the WIP patch.

[Julien Chaffraix]

(In reply to comment #5)
> Well, before r110072, the height of the shadow DIV (innerTextElement() in RenderTextControlSingleLine) was 4px because the height of <input> is 10px and borders and paddings were subtracted from 10px.

I think this is a wrong interpretation of the dump: your div was 13px tall. The shadow DIV box's height was 4px but it had 9px of layout overflow (as show by the 13px scrollHeight on the layer). It was clipped at 4px though which is what you seem to expect.

> Since r110072, the height of the shadow DIV is 13px.

The change didn't change the full height of the div (it's not a layout change).

> It should be 4px and I don't know why r110072 changed this.

I would expect that it exposed a flaw in the logic inside RenderTextControlSingleLine (likely layout but it could be somewhere else).

Btw, I really don't understand why 4px is the one and only one value allowed for the logical height.

[Julien Chaffraix]

> Btw, I really don't understand why 4px is the one and only one value allowed for the logical height.

If I had spend 3 minutes debugging, I would have seen that this is 10px - 6px for borders and paddings per our default UA style sheet.

I think I have found the issue:

http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderTextControlSingleLine.cpp#L232

When querying the height(), layout has not been called on |innerTextRenderer| which makes RenderTextControlSingleLine's layout algorithm pretty random between 2 calls as we are using the previous layout's height.

For some reason, r110072 happened to make it pick the wrong result.

[Julien Chaffraix]

Created attachment 133422 [details]
Real fix, properly do a 2 layout pass.

[WebKit Review Bot]

Comment on attachment 133422 [details]
Real fix, properly do a 2 layout pass.

Attachment 133422 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12120480

New failing tests:
fast/table/colspanMinWidth-vertical.html
fast/repaint/search-field-cancel.html

[Julien Chaffraix]

Created attachment 133489 [details]
Same as previously but added a progression, the EWS should be happy now

[Tony Chang]

Comment on attachment 133489 [details]
Same as previously but added a progression, the EWS should be happy now

This patch seems more correct, although it does mean we may layout twice in some cases.

Julien, maybe you can wait on landing until Monday to give others a chance to review or explain why we shouldn't do 2 layouts here.

[Ojan Vafai]

Comment on attachment 133489 [details]
Same as previously but added a progression, the EWS should be happy now

This looks to me like a vertical-writing-mode bug. I think the problem here is that RenderTextControlSingleLine::layout is mixing logical and physical values. It should only deal with logical values. If you s/height/logicalHeight and s/width/logicalWidth everywhere in this function, does that fix the bug?

[Julien Chaffraix]

> This looks to me like a vertical-writing-mode bug. I think the problem here is that RenderTextControlSingleLine::layout is mixing logical and physical values. It should only deal with logical values.

I totally second your analysis of the image change (which is a progression even if we are still broken as we don't follow the writing-mode).

However I am not trying to fix RenderTextSingleControl to work with vertical writing modes as part of this bug. I am merely trying to make the layout algorithm return consistent results over several calls (which is not the case at the moment due to the fact that we rely on the previous layout's information).

> If you s/height/logicalHeight and s/width/logicalWidth everywhere in this function, does that fix the bug?

I haven't tried but I bet the answer is 'no' as this is not a writing mode issue.

[Adele Peterson]

2 pass layout seems fine to me (and hyatt concurs).

[Kent Tamura]

I'd like to say r+.  We don't need a complete fix for the vertical writing mode for now.

[Ojan Vafai]

Comment on attachment 133489 [details]
Same as previously but added a progression, the EWS should be happy now

I see. Seems fine.

[Julien Chaffraix]

Created attachment 133824 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 133824 [details]
Patch for landing

Clearing flags on attachment: 133824

Committed r112113: <http://trac.webkit.org/changeset/112113>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to comment #18)
> (From update of attachment 133824 [details])
> Clearing flags on attachment: 133824
> 
> Committed r112113: <http://trac.webkit.org/changeset/112113>

fast/forms/textfield-overflow.html is failing in Lion after this change (e.g. <http://build.webkit.org/results/Lion%20Intel%20Debug%20(WebKit2%20Tests)/r112114%20(5361)/fast/forms/textfield-overflow-pretty-diff.html>).

(In reply to comment #20)
> (In reply to comment #18)
> > (From update of attachment 133824 [details] [details])
> > Clearing flags on attachment: 133824
> > 
> > Committed r112113: <http://trac.webkit.org/changeset/112113>
> 
> fast/forms/textfield-overflow.html is failing in Lion after this change (e.g. <http://build.webkit.org/results/Lion%20Intel%20Debug%20(WebKit2%20Tests)/r112114%20(5361)/fast/forms/textfield-overflow-pretty-diff.html>).

Sorry, I should point out that the failure is happening in WebKit2, and I see that mac-wk2 has its own expected results for this test.

Removed the Mac WebKit2-specific expected results in <http://trac.webkit.org/r112128>.

[Julien Chaffraix]

(In reply to comment #22)
> Removed the Mac WebKit2-specific expected results in <http://trac.webkit.org/r112128>.

Great, thanks Dan. Sorry, I have forgotten to update Mac WebKit2 expectations. The new result is consistent with what we expect here (4px height div with some scroll height).