Bug 6844 - elementAtPoint returns the list when the point is over a list marker
Summary: elementAtPoint returns the list when the point is over a list marker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Justin Garcia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-01-26 10:54 PST by Justin Garcia
Modified: 2006-03-06 13:26 PST (History)
1 user (show)

See Also:


Attachments
patch (6.08 KB, patch)
2006-02-21 21:29 PST, Justin Garcia
justin.garcia: review-
Details | Formatted Diff | Diff
patch (12.46 KB, patch)
2006-02-23 14:08 PST, Justin Garcia
no flags Details | Formatted Diff | Diff
patch (16.16 KB, patch)
2006-02-23 14:09 PST, Justin Garcia
no flags Details | Formatted Diff | Diff
patch (15.96 KB, patch)
2006-02-23 16:32 PST, Justin Garcia
no flags Details | Formatted Diff | Diff
patch (13.91 KB, patch)
2006-03-02 21:04 PST, Justin Garcia
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 2006-01-26 10:54:49 PST
elementAtPoint returns the list when the point is over a list marker.  It would make more sense if it returned the list item associated with the list marker.
Comment 1 Justin Garcia 2006-01-26 11:32:18 PST
Also in radar: <rdar://problem/4391563>
Comment 2 Justin Garcia 2006-02-21 21:29:04 PST
Created attachment 6656 [details]
patch

The position and size of the list marker's renderer doesn't correspond to the position and size of the marker that's painted.  So, I save the rect where the marker is painted, and that's where I do hit testing.  Want to show this to hyatt while I work on creating layout tests.
Comment 3 Dave Hyatt 2006-02-22 12:12:25 PST
Comment on attachment 6656 [details]
patch

(1) I'd only do this special nodeAtPoint call if your list-style-position is outside and if hittestaction is foreground.

(2) I'd just make setInnerNode public.

Make sure you test both inside and outside markers.  Inside markers should already work, which is why I'd make this patch only for outside markers.

(I like clearly identifying the "outside" hacks in the code, so that when we ultimately fix them, we'll know what to patch.)
Comment 4 Darin Adler 2006-02-23 08:41:08 PST
Hyatt told me that while he thinks this is OK as a short-term test, the longer-term fix should be to set the position and size of the marker render object properly, and we should remove this hack we just had to add. I think that someone should look at the longer-term fix soon.
Comment 5 Justin Garcia 2006-02-23 12:30:25 PST
Comment on attachment 6656 [details]
patch

I'm clearing the review flag for now, since I need to make two changes to this patch.  

First, If text that is inside the list item overlaps the list marker (imagine a <span> with a negative margin inside the list item), I want nodeAtPoint to return the text, not the list marker.  So, in RenderListItem::nodeAtPoint, I should only call RenderListMarker::nodeAtPoint if normal hit testing fails.  

Also, since m_rect is cached at paint time, a normal layout test won't be able to test these changes.  I'll have to pull the code that computes the size and position of the list marker out into a separate function and make sure it's called from nodeAtPoint if the cached rect is empty.
Comment 6 Justin Garcia 2006-02-23 14:08:31 PST
Created attachment 6685 [details]
patch
Comment 7 Justin Garcia 2006-02-23 14:09:53 PST
Created attachment 6686 [details]
patch
Comment 8 Justin Garcia 2006-02-23 16:32:39 PST
Created attachment 6689 [details]
patch
Comment 9 Justin Garcia 2006-02-23 16:34:08 PST
Comment on attachment 6689 [details]
patch

I made the two changes that I mentioned above.
Comment 10 Darin Adler 2006-02-26 08:57:01 PST
Comment on attachment 6689 [details]
patch

+    bool insideMarker = m_marker->nodeAtPoint(i, x, y, tx + m_x, ty + m_y, hitTestAction);
+    if (insideMarker)
+        setInnerNode(i);
+    return insideMarker;

To match the logic above, I'd do instead:

+    if (!m_marker->nodeAtPoint(i, x, y, tx + m_x, ty + m_y, hitTestAction)
+        return false;
+
+    setInnerNode(i);
+    return true;

But obviously the code is OK as-is, just a suggestion.

RenderListMarker::nodeAtPoint does not set the innerNode when it returns true. That means that if anyone other than RenderListItem::nodeAtPoint calls it, it won't work properly. Either it should call setInnerNode or it should not be called nodeAtPoint.

The patch otherwise looks good. I'm going to say r=me, because the nodeAtPoint quibble is a minor one.
Comment 11 Justin Garcia 2006-02-27 16:39:06 PST
Landed with the changes darin suggested + some small changes.
Comment 12 mitz 2006-03-02 10:24:27 PST
I think the fix caused bug 7542.
Comment 13 Justin Garcia 2006-03-02 20:49:20 PST
elementAtPoint uses NodeInfo's innerNonSharedNode, which the previous checkin didn't set.  I'm reopening this bug.
Comment 14 Justin Garcia 2006-03-02 21:04:51 PST
Created attachment 6819 [details]
patch

RenderListMarker::nodeAtPoint was only setting NodeInfo's innerNode.  That was enough to get the list item's mouse events firing over the list marker, but not enough to fix elementAtPoint, since elementAtPoint uses NodeInfo's innerNonSharedNode to populate the dictionary that it returns.

This change calls RenderObject::setInnerNode(), which handles setting both the innerNode and the innerNonSharedNode.  Since RenderObject::setInnerNode() assumes that it is being called from the renderer who's element is the inner node, I needed to call it from inside RenderListItem::nodeAtPoint.

elementAtPoint is used to determine the drag operation that will be performed when something is dropped over a given point.  So, to test my fix, I wrote a test that drags some text and drops it over the list marker.  This required three fixes to DumpRenderTree:
First, when I added DumpRenderTree's WebView to a window, I didn't have the eventSender send the window number along with the event.
Second, I always call performDragOperation on the destination in DumpRenderTree.  If the WebView doesn't want to perform a drag operation, it won't, but it needs to be asked to try or else it gets into a bad state and asserts fire.
Third, dragging tests in particular, and tests that use the eventSender in general, often need to pause in order for some series of mouse moves and clicks to register as a particular operation.  A drag operation will only kick off for example, if the mouse is held down for a split second on the selected content, otherwise it will turn into a create-selection operation.  I didn't want to add explicit delays to layout tests (1 full second is needed after a double click to make the next mousedown a drag-start and not a triple-click!).  So, I added a leapForward method to the eventSender, that adds the given offset to the timestamp of each event sent thereafter.
Comment 15 Justin Garcia 2006-03-02 21:20:14 PST
The leapForward bit is kind of speculative..I think that timestamps are only ever compared to other timestamps, but if they are ever compared to the current time, sending an event with a timestamp in the future could cause problems.
Comment 16 Justin Garcia 2006-03-02 21:21:36 PST
(In reply to comment #15)
> I think that timestamps are only ever compared to other timestamps,

by 'other' timestamps I meant timestamps of past events.
Comment 17 Darin Adler 2006-03-03 06:47:04 PST
Comment on attachment 6819 [details]
patch

r=me