Summary: | Text selection in text area in auto scroll mode goes wrong. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Misha <mtutunik> | ||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | adele, ap, darin, enrica, eric, jamesr, jchaffraix, max.hong.shen, mifenton, mitz, ojan, rniwa, simon.fraser, sukolsak, tkent, webkit.review.bot, yael | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac (Intel) | ||||||||||
OS: | OS X 10.7 | ||||||||||
Bug Depends on: | 94076 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Misha
2011-12-12 14:52:48 PST
Created attachment 157713 [details]
Patch
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. Created attachment 158495 [details]
Patch
(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 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. Created attachment 158497 [details]
Patch
(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 on attachment 158497 [details] Patch Clearing flags on attachment: 158497 Committed r125648: <http://trac.webkit.org/changeset/125648> All reviewed patches have been landed. Closing bug. |