RESOLVED FIXED Bug 14653
REGRESSION (r23994): No caret is drawn after clicking a search field's placeholder text
https://bugs.webkit.org/show_bug.cgi?id=14653
Summary REGRESSION (r23994): No caret is drawn after clicking a search field's placeh...
mitz
Reported 2007-07-18 00:39:31 PDT
I noticed that sometimes (about half the time) when I click the search field on the right hand side of the page in the URL, the search field gets focus and I can type into it, but the caret is not showing.
Attachments
Patch idea (1.64 KB, patch)
2007-07-27 11:10 PDT, mitz
no flags
Patch, including change log and layout test (5.02 KB, patch)
2007-07-28 04:40 PDT, mitz
darin: review+
David Kilzer (:ddkilzer)
Comment 1 2007-07-18 23:06:58 PDT
I can't reproduce this with a local debug build of WebKit r24439 with Safari 3.0 (522.12) on Mac OS X 10.4.10 (8R218). Is it the search field in the top nav bar, or the search field within the content of the page?
mitz
Comment 2 2007-07-18 23:12:32 PDT
(In reply to comment #1) > Is it the search field in the top > nav bar, or the search field within the content of the page? The one within the content of the page.
mitz
Comment 3 2007-07-19 12:43:01 PDT
Reduction: <input type="search" placeholder="Click this text"> Click the placeholder text, not the space to the right, to see the bug. Regressed in <http://trac.webkit.org/projects/webkit/changeset/23994> (note that this is not the regression that was fixed in r24003). The problem is that computing the target element lazily breaks when the target node is moved around during event dispatch. By the time handleMousePressEventSingleClick() sees the event and looks for the target node, it gets the renderer-less text node instead of the search field's inner div element.
mitz
Comment 4 2007-07-19 14:47:36 PDT
The fact that this worked before r23994 is just a coincidence. In fact, if you try to cache the "target element" at MouseEventWithHitTestResults construction time, you also get it wrong, because the text node is still in the document at that point. It was really the gap between when the target element was cached and when the target node was computed -- a gap during which the text node had been removed from the tree -- that made it work prior to r23994. While the old behavior is easy to reconstruct, the correct fix would be for the placeholder text never to be returned from nodeAtPoint. The fact that placeholder text is implemented using a text node should remain private to RenderTextControl.
Matt Lilek
Comment 5 2007-07-27 00:25:50 PDT
*** Bug 14775 has been marked as a duplicate of this bug. ***
mitz
Comment 6 2007-07-27 11:10:39 PDT
Created attachment 15707 [details] Patch idea This works (without checking if the field is currently displaying placeholder text or not) because the inner text div cannot contain inline elements. Since positionForCoordinates is not patched, you can still select text etc. Not directly related to the regression, I noticed that the behavior in TOT of drag and drop into a text field with placeholder text is funny: it draws a drop caret in the placeholder text. I am not sure what it should do.
mitz
Comment 7 2007-07-28 04:40:55 PDT
Created attachment 15717 [details] Patch, including change log and layout test Perhaps not a very elegant solution, but the least intrusive that I could think of.
Darin Adler
Comment 8 2007-07-30 11:51:59 PDT
Comment on attachment 15717 [details] Patch, including change log and layout test + virtual ~RenderTextControlInnerBlock() { } There's no need to declare that explicitly. r=me
Mark Rowe (bdash)
Comment 9 2007-08-03 07:30:59 PDT
Mark Rowe (bdash)
Comment 10 2007-08-03 07:34:10 PDT
Landed in r24842.
Mark Rowe (bdash)
Comment 11 2007-08-03 11:51:57 PDT
r24850 was a followup fix for the Windows build.
Note You need to log in before you can comment on or make changes to this bug.