Bug 80531

Summary: REGRESSION(r110072): fast/forms/textfield-overflow.html is failing
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, dglazkov, hyatt, mitz, senorblanco, tkent, tony, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
WIP fix, need to think about it / some feedbacks.
none
Real fix, properly do a 2 layout pass.
none
Same as previously but added a progression, the EWS should be happy now
none
Patch for landing none

Description Julien Chaffraix 2012-03-07 12:27:36 PST
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.
Comment 1 Julien Chaffraix 2012-03-12 13:07:25 PDT
Created attachment 131387 [details]
WIP fix, need to think about it / some feedbacks.
Comment 2 Julien Chaffraix 2012-03-12 13:09:37 PDT
Adding people who have touched this code and could have good comments on the approach :-}
Comment 3 Kent Tamura 2012-03-13 19:20:20 PDT
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?
Comment 4 Julien Chaffraix 2012-03-13 19:43:50 PDT
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.
Comment 5 Kent Tamura 2012-03-20 23:24:17 PDT
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.
Comment 6 Julien Chaffraix 2012-03-22 16:27:32 PDT
(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.
Comment 7 Julien Chaffraix 2012-03-22 18:09:01 PDT
> 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.
Comment 8 Julien Chaffraix 2012-03-22 20:32:42 PDT
Created attachment 133422 [details]
Real fix, properly do a 2 layout pass.
Comment 9 WebKit Review Bot 2012-03-22 21:28:34 PDT
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
Comment 10 Julien Chaffraix 2012-03-23 08:24:57 PDT
Created attachment 133489 [details]
Same as previously but added a progression, the EWS should be happy now
Comment 11 Tony Chang 2012-03-23 14:25:28 PDT
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.
Comment 12 Ojan Vafai 2012-03-23 14:55:16 PDT
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?
Comment 13 Julien Chaffraix 2012-03-23 15:18:05 PDT
> 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.
Comment 14 Adele Peterson 2012-03-23 15:34:36 PDT
2 pass layout seems fine to me (and hyatt concurs).
Comment 15 Kent Tamura 2012-03-23 16:25:40 PDT
I'd like to say r+.  We don't need a complete fix for the vertical writing mode for now.
Comment 16 Ojan Vafai 2012-03-23 18:15:32 PDT
Comment on attachment 133489 [details]
Same as previously but added a progression, the EWS should be happy now

I see. Seems fine.
Comment 17 Julien Chaffraix 2012-03-26 09:05:47 PDT
Created attachment 133824 [details]
Patch for landing
Comment 18 WebKit Review Bot 2012-03-26 09:46:35 PDT
Comment on attachment 133824 [details]
Patch for landing

Clearing flags on attachment: 133824

Committed r112113: <http://trac.webkit.org/changeset/112113>
Comment 19 WebKit Review Bot 2012-03-26 09:46:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 mitz 2012-03-26 10:50:16 PDT
(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>).
Comment 21 mitz 2012-03-26 10:51:31 PDT
(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.
Comment 22 mitz 2012-03-26 10:56:27 PDT
Removed the Mac WebKit2-specific expected results in <http://trac.webkit.org/r112128>.
Comment 23 Julien Chaffraix 2012-03-26 10:58:54 PDT
(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).