Bug 74346 - Text selection in text area in auto scroll mode goes wrong.
Summary: Text selection in text area in auto scroll mode goes wrong.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 94076
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-12 14:52 PST by Misha
Modified: 2012-08-15 01:06 PDT (History)
17 users (show)

See Also:


Attachments
Patch (9.15 KB, patch)
2012-08-10 05:29 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (8.85 KB, patch)
2012-08-14 21:08 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (8.76 KB, patch)
2012-08-14 21:29 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Misha 2011-12-12 14:52:48 PST
In order to reproduce have a text area filled with a text long enough to be able to scroll the text area up and down.

- Scroll the text area down to the end.
- Slowly start dragging your mouse starting from the some place in the line closets to the upper edge of the text area.
- Once the contents of the text area get scrolled a little notice that the selection jumped down to the end.

The selection is not following the current scroll position of the text. If mouse is moved slightly down auto scrolling stops and selection is getting updated to the correct position. If mouse position is not changed then when left button is released the end of the selection can be either at the end of the text or closer to the correct position (where mouse down happened) depending on how much scrolling was done.



When auto scrolling is on updateSelectionForMouseDrag() is invoked. Inside we call:
VisiblePosition targetPosition = selectionExtentRespectingEditingBoundary(m_frame->selection()->selection(), hitTestResult.localPoint(), target);
From what I could see when we start dragging mouse up the offset of the targetPosition is pointing to the end of the text. 

Reproducible on Mac with Chrome version 15.0.874.121

It might me the same issue as https://bugs.webkit.org/show_bug.cgi?id=31581
Comment 1 Sukolsak Sakshuwong 2012-08-10 05:29:03 PDT
Created attachment 157713 [details]
Patch
Comment 2 Ojan Vafai 2012-08-14 19:01:39 PDT
Comment on attachment 157713 [details]
Patch

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

R- for the 100ms timeout. The C++ side looks good to me.

> LayoutTests/fast/events/autoscroll-in-textarea.html:57
> +        var x = textarea.offsetLeft + 20;
> +        var y = textarea.offsetTop + 8;

Can you make these less magic? For x, you can do textarea.offsetLeft + textarea.offsetWidth / 2.

For y, would adding 1 be enough?

> LayoutTests/fast/events/autoscroll-in-textarea.html:63
> +    setTimeout(autoscrollTestPart2, 100);

Do you need this 100ms? 0 should be sufficient I would think. No need to make the test take ~100ms longer.
Comment 3 Sukolsak Sakshuwong 2012-08-14 21:08:58 PDT
Created attachment 158495 [details]
Patch
Comment 4 Sukolsak Sakshuwong 2012-08-14 21:10:25 PDT
(In reply to comment #2)
> (From update of attachment 157713 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157713&action=review
> 
> R- for the 100ms timeout. The C++ side looks good to me.
> 
> > LayoutTests/fast/events/autoscroll-in-textarea.html:57
> > +        var x = textarea.offsetLeft + 20;
> > +        var y = textarea.offsetTop + 8;
> 
> Can you make these less magic? For x, you can do textarea.offsetLeft + textarea.offsetWidth / 2.
> 
> For y, would adding 1 be enough?
> 
> > LayoutTests/fast/events/autoscroll-in-textarea.html:63
> > +    setTimeout(autoscrollTestPart2, 100);
> 
> Do you need this 100ms? 0 should be sufficient I would think. No need to make the test take ~100ms longer.

Fixed. Thank you for the comments.
Comment 5 Ojan Vafai 2012-08-14 21:18:21 PDT
Comment on attachment 158495 [details]
Patch

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

Looks great. Please address my last comments on the test and then feel free to commit.

Just to be sure, you verified that the test still fails without your fix and without the setTimeout, right? :)

> LayoutTests/fast/events/autoscroll-in-textarea.html:1
> +<!doctype html>

Nit: typically we uppercase doctype:
<!DOCTYPE html>

> LayoutTests/fast/events/autoscroll-in-textarea.html:42
> +    testRunner.waitUntilDone();

Now that you don't have the setTimeout, you don't need this line.

> LayoutTests/fast/events/autoscroll-in-textarea.html:69
> +if (window.testRunner)
> +    testRunner.notifyDone();

and without the waitUntilDone call above, you don't need to notifyDone either.
Comment 6 Sukolsak Sakshuwong 2012-08-14 21:29:59 PDT
Created attachment 158497 [details]
Patch
Comment 7 Sukolsak Sakshuwong 2012-08-14 21:33:11 PDT
(In reply to comment #5)
> (From update of attachment 158495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158495&action=review
> 
> Looks great. Please address my last comments on the test and then feel free to commit.
> 
> Just to be sure, you verified that the test still fails without your fix and without the setTimeout, right? :)

Yes, I've verified that case.

> > LayoutTests/fast/events/autoscroll-in-textarea.html:1
> > +<!doctype html>
> 
> Nit: typically we uppercase doctype:
> <!DOCTYPE html>
> 
> > LayoutTests/fast/events/autoscroll-in-textarea.html:42
> > +    testRunner.waitUntilDone();
> 
> Now that you don't have the setTimeout, you don't need this line.
> 
> > LayoutTests/fast/events/autoscroll-in-textarea.html:69
> > +if (window.testRunner)
> > +    testRunner.notifyDone();
> 
> and without the waitUntilDone call above, you don't need to notifyDone either.

Done. Thank you!
Comment 8 WebKit Review Bot 2012-08-14 22:21:02 PDT
Comment on attachment 158497 [details]
Patch

Clearing flags on attachment: 158497

Committed r125648: <http://trac.webkit.org/changeset/125648>
Comment 9 WebKit Review Bot 2012-08-14 22:21:08 PDT
All reviewed patches have been landed.  Closing bug.