Bug 38506 - Regression (r41792): Repaint issue with textarea inside overflow:scroll div
Summary: Regression (r41792): Repaint issue with textarea inside overflow:scroll div
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2010-05-03 21:51 PDT by Steven Lai
Modified: 2010-06-03 15:08 PDT (History)
6 users (show)

See Also:


Attachments
Test case (441 bytes, text/html)
2010-05-03 21:51 PDT, Steven Lai
no flags Details
Test case that doesn't involved textareas (644 bytes, text/html)
2010-05-07 15:08 PDT, James Robinson
no flags Details
Patch (52.55 KB, patch)
2010-05-07 17:11 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (40.97 KB, patch)
2010-06-02 19:32 PDT, James Robinson
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steven Lai 2010-05-03 21:51:27 PDT
Created attachment 54993 [details]
Test case

Refer to the attached HTML.

There is a textarea inside 2 nested divs, both being position:absolute. Typing text into the textarea results in corrupted text when and only when either scrollLeft or scrollTop of the outer div are > 10.

See the attached screen cap for the corrupted text.

* STEPS TO REPRODUCE
1. Load the html in Safari 4+ or Webkit
2. Type some text into text area

* RESULTS
Text corrupted.

* REGRESSION
Since r41792
http://trac.webkit.org/changeset/41792

* NOTES
If both scrollLeft and scrollTop are < 10, the text is only slightly corrupted.
Already typed texts will be recovered when being repainted, after further scrolling or switching to another window.
Comment 1 mitz 2010-05-03 21:54:38 PDT
<rdar://problem/7100751>

The layout state at the time of repaint has an incorrect offset. It’s a subtree layout and the LayoutState::LayoutState(RenderObject*) constructor, called with the textarea as the root, does not account for the scroll offset of the container (the div).
Comment 2 James Robinson 2010-05-07 15:08:40 PDT
Created attachment 55421 [details]
Test case that doesn't involved textareas

This test case shows that the issue is not limited to text areas but any object relayout boundary.
Comment 3 James Robinson 2010-05-07 15:10:56 PDT
The second test case fails at r41760.  I think that the textarea behavior did regress at r41792 but the underlying bug (the LayoutState ignores the scrolloffset) predates that.
Comment 4 James Robinson 2010-05-07 17:11:38 PDT
Created attachment 55437 [details]
Patch
Comment 5 mitz 2010-06-02 11:35:58 PDT
Comment on attachment 55437 [details]
Patch

> +        RenderLayer* layer = toRenderBoxModelObject(container)->layer();

Although inspection of the current code shows that it’s true, there is no great guarantee that container is a RenderBoxModelObject. Do you think we can change the signature of the constructor to require a RenderBoxModelObject?

Should this also initialize m_clipped and m_clipRect?
Comment 6 James Robinson 2010-06-02 17:50:36 PDT
Thanks for taking a look at this!  I had actually forgotten about this patch completely.

I'll look in to whether to initialize m_clipped and m_clipRect explicitly.  I'm not sure what you mean about changing the signature - do you mean making the constructor something like:

LayoutState(RenderObject* root, RenderBoxModelObject* container)

?  That would just move the cast to the callsite, I believe.
Comment 7 James Robinson 2010-06-02 19:32:23 PDT
Created attachment 57727 [details]
Patch
Comment 8 James Robinson 2010-06-02 19:33:56 PDT
Updated the patch to properly take care of m_clipped and m_clipRect and added a test to cover it.  Without the clip handling we would overpaint in some circumstances.  layout-state-scrolloffset3.html covers this case.

I'm still not quite sure what you mean by changing the signature.
Comment 9 mitz 2010-06-02 20:01:10 PDT
(In reply to comment #6)
> I'm not sure what you mean about changing the signature - do you mean making the constructor something like:
> 
> LayoutState(RenderObject* root, RenderBoxModelObject* container)
> 
> ?  That would just move the cast to the callsite, I believe.

Or perhaps the caller already has a RenderBoxModelObject, or perhaps the caller’s caller, or perhaps just casting there would make it more obvious why the cast is correct; and changing the signature would force new callers to do the right thing.
Comment 10 James Robinson 2010-06-03 15:08:14 PDT
Committed r60640: <http://trac.webkit.org/changeset/60640>