Bug 14653 - REGRESSION (r23994): No caret is drawn after clicking a search field's placeholder text
Summary: REGRESSION (r23994): No caret is drawn after clicking a search field's placeh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL: http://discussions.apple.com/forum.js...
Keywords: HasReduction, InRadar, Regression
: 14775 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-07-18 00:39 PDT by mitz
Modified: 2007-08-03 11:51 PDT (History)
1 user (show)

See Also:


Attachments
Patch idea (1.64 KB, patch)
2007-07-27 11:10 PDT, mitz
no flags Details | Formatted Diff | Diff
Patch, including change log and layout test (5.02 KB, patch)
2007-07-28 04:40 PDT, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 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.
Comment 1 David Kilzer (:ddkilzer) 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?

Comment 2 mitz 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.
Comment 3 mitz 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.
Comment 4 mitz 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.
Comment 5 Matt Lilek 2007-07-27 00:25:50 PDT
*** Bug 14775 has been marked as a duplicate of this bug. ***
Comment 6 mitz 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.
Comment 7 mitz 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.
Comment 8 Darin Adler 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
Comment 9 Mark Rowe (bdash) 2007-08-03 07:30:59 PDT
<rdar://problem/5383841>
Comment 10 Mark Rowe (bdash) 2007-08-03 07:34:10 PDT
Landed in r24842.
Comment 11 Mark Rowe (bdash) 2007-08-03 11:51:57 PDT
r24850 was a followup fix for the Windows build.