Bug 9942 - REGRESSION: Editing text on line numbers greater than 'rows' attribute makes native text area scroll up
Summary: REGRESSION: Editing text on line numbers greater than 'rows' attribute makes ...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Adele Peterson
Keywords: InRadar, Regression
Depends on:
Reported: 2006-07-15 14:49 PDT by David Kilzer (:ddkilzer)
Modified: 2006-07-23 10:24 PDT (History)
4 users (show)

See Also:

potential fix (2.69 KB, patch)
2006-07-22 22:10 PDT, Adele Peterson
no flags Details | Formatted Diff | Diff
another patch (2.89 KB, patch)
2006-07-23 00:38 PDT, Adele Peterson
no flags Details | Formatted Diff | Diff
patch w/ changelog & test (12.34 KB, patch)
2006-07-23 01:31 PDT, Adele Peterson
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2006-07-15 14:49:51 PDT
Steps to reproduce:

1. Open a web page with a text area (like this bug).
2. Enter line numbers on each line, then hit Enter, so that there are twice as many lines as the 'rows' attribute for the text area is set to.  (For Bugzilla comments, this would be 20 lines total.)
3. Move the cursor to Line 15, but make sure Line 15 is NOT the last visible line in the text area.  (Clicking to the right of the "15" only works if you're immediately next to the "5".  See Bug 9936.)
4. Type any character, like "a".

Expected results:

The text area does not scroll up.

Actual results:

The text area scrolls up so that the line being edited is at the bottom of the visible area.


This works as expected on production Safari 2.0.4 (419.3) on Mac OS X 10.4.7 (8J135/PowerPC).


Reproduced on a locally-built WebKit r15458 with the above versions of Safari and Mac OS X.

This may be related to Bug 9581.
Comment 1 Darin Adler 2006-07-22 21:40:20 PDT
This sounds like a bug Adele is already working on.

Comment 2 Adele Peterson 2006-07-22 22:10:00 PDT
Created attachment 9617 [details]
potential fix

I don't think this is the right fix for the long term, but in the short term, this fixes the textarea case.  For example, if a div w/ overflow were nested in multiple RenderBlocks/RenderFlexibleBoxes, then this fix wouldn't apply since it only checks the direct parent.  It also wouldn't work for children that were RenderFlexibleBoxes.  It is a lower risk fix than some of the other ideas that hyatt, darin, and i discussed.
Comment 3 Darin Adler 2006-07-22 22:12:30 PDT
Comment on attachment 9617 [details]
potential fix

Even if we're doing this hack, I don't think that a two-pass layout system justifies having a counter for number of layouts. How about just a boolean that's true when you're in the first pass?
Comment 4 Adele Peterson 2006-07-23 00:21:48 PDT
I tried generalizing this fix a little more, and it seems that horizontal flex boxes (like in a text field) don't always cause the child to layout twice.  So the more general version fails.
Comment 5 Adele Peterson 2006-07-23 00:38:34 PDT
Created attachment 9618 [details]
another patch

this doesn't change anything for horizontal boxes.
Comment 6 Adele Peterson 2006-07-23 01:31:17 PDT
Created attachment 9620 [details]
patch w/ changelog & test
Comment 7 Maciej Stachowiak 2006-07-23 01:33:43 PDT
Comment 8 Maciej Stachowiak 2006-07-23 01:43:56 PDT
Comment on attachment 9620 [details]
patch w/ changelog & test

I think this is good enough for right now.
Comment 9 Adele Peterson 2006-07-23 01:46:51 PDT
Comment 10 Darin Adler 2006-07-23 08:31:25 PDT
>    if (view()->flexBoxInFirstLayout() == this) 
>        view()->setFlexBoxInFirstLayout(0); 
>    else 
>        view()->setFlexBoxInFirstLayout(this); 

Adele, the code above won't work right for nested flex boxes. You should do this instead:

>    if (view()->flexBoxInFirstLayout() == this) 
>        view()->setFlexBoxInFirstLayout(0); 
>    else if (!view()->flexBoxInFirstLayout())
>        view()->setFlexBoxInFirstLayout(this); 

which will work for the nested case too.
Comment 11 Adele Peterson 2006-07-23 10:24:23 PDT
updated w/ darin's suggested change => r15582.