Bug 6844 – elementAtPoint returns the list when the point is over a list marker

• Component: HTML Editing
• Status: RESOLVED FIXED

[Justin Garcia]

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.

[Justin Garcia]

Also in radar: <rdar://problem/4391563>

[Justin Garcia]

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.

[Dave Hyatt]

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.)

[Darin Adler]

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.

[Justin Garcia]

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.

[Justin Garcia]

Created attachment 6685 [details]
patch

[Justin Garcia]

Created attachment 6686 [details]
patch

[Justin Garcia]

Created attachment 6689 [details]
patch

[Justin Garcia]

Comment on attachment 6689 [details]
patch

I made the two changes that I mentioned above.

[Darin Adler]

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.

[Justin Garcia]

Landed with the changes darin suggested + some small changes.

I think the fix caused bug 7542.

[Justin Garcia]

elementAtPoint uses NodeInfo's innerNonSharedNode, which the previous checkin didn't set.  I'm reopening this bug.

[Justin Garcia]

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.

[Justin Garcia]

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.

[Justin Garcia]

(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.

[Darin Adler]

Comment on attachment 6819 [details]
patch

r=me