Bug 24726

Summary: hit testing doesn't work right when the click is on anonymous content
Product: WebKit Reporter: Darin Adler <darin>
Component: HTML EditingAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, ojan, simon.fraser
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
first cut at a test, made by Eric
none
patch adele: review+

Darin Adler
Reported 2009-03-20 14:23:17 PDT
When looking at a bug affecting Apple’s Mail application I discovered that hit testing doesn’t work when the item hit (or the nearest item) is anonymous content. While I can’t share the Mail test case here, we can make other test cases.
Attachments
first cut at a test, made by Eric (196 bytes, text/html)
2009-03-20 14:54 PDT, Darin Adler 		no flags
Details
patch (19.27 KB, patch)
2009-03-20 15:48 PDT, Darin Adler 		adele: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2009-03-20 14:54:57 PDT
Created attachment 28802 [details] first cut at a test, made by Eric
Darin Adler
Comment 2 2009-03-20 15:22:55 PDT
rdar://problem/6696992
Darin Adler
Comment 3 2009-03-20 15:48:56 PDT
Created attachment 28805 [details] patch
Ojan Vafai
Comment 4 2009-03-20 16:28:44 PDT
Comment on attachment 28805 [details] patch > Index: WebCore/rendering/RenderObject.cpp > =================================================================== > --- WebCore/rendering/RenderObject.cpp (revision 41868) > +++ WebCore/rendering/RenderObject.cpp (working copy) > +VisiblePosition RenderObject::createVisiblePosition(int offset, EAffinity affinity) > +{ > + // If is is a non-anonymous renderer, then it's simple. > + if (Node* node = this->node()) > + return VisiblePosition(node, offset, affinity); > + > + // Find a nearby non-anonymous renderer. > + RenderObject* child = this; > + while (RenderObject* parent = child->parent()) { I think you need to check here that the parent is of the same editability as the child like in positionForPointRespectingEditingBoundaries. > + // Find non-anonymous content after. > + RenderObject* renderer = child; > + while ((renderer = renderer->nextInPreOrder(parent))) { > + if (Node* node = renderer->node()) > + return VisiblePosition(node, 0, DOWNSTREAM); > + } Maybe add the following test cases to test the rest of this? -Editable element with generated content only at the beginning -Editable element with only generated content inside > + // Find non-anonymous content before. > + renderer = child; > + while ((renderer = renderer->previousInPreOrder())) { > + if (renderer == parent) > + break; > + if (Node* node = renderer->node()) > + return VisiblePosition(node, numeric_limits<int>::max(), DOWNSTREAM); > + } > + > + // Use the parent itself unless it too is anonymous. > + if (Node* node = parent->node()) > + return VisiblePosition(node, 0, DOWNSTREAM); > + > + // Repeat at the next level up. > + child = parent; > + } > + Is this actually possible? If so, would be good to have a test case. Maybe this even deserves an ASSERT(false)? > + // Everything was anonymous. Give up. > + return VisiblePosition(); > +} > + > +VisiblePosition RenderObject::createVisiblePosition(const Position& position) > +{ > + if (position.container) > + return VisiblePosition(position); > + > + ASSERT(!node()); > + return createVisiblePosition(0, DOWNSTREAM); > +} > Index: LayoutTests/editing/selection/hit-test-anonymous.html > =================================================================== > --- LayoutTests/editing/selection/hit-test-anonymous.html (revision 0) > +++ LayoutTests/editing/selection/hit-test-anonymous.html (revision 0) > @@ -0,0 +1,74 @@ > +<head> > +<style> > +span:before { > + content: "<before> "; > +} > +span:after { > + content: " <after>"; > +} > +</style> > +<script> Not a big deal, but LayoutTests/editing/selection/resources/js-test-selection-shared.js already has equivalents to the first three of these methods. Although it's DOM-based in it's asserts rather than string-based and it assumes you pull in js-test-pre.js. > + function nodeAsString(node) > + { > + if (node && node.nodeType == Node.TEXT_NODE) > + return "text in " + nodeAsString(node.parentNode); > + if (node && node.nodeType == Node.ELEMENT_NODE) { > + var id; > + if (id = node.getAttribute("id")) > + return id; > + } > + return node; > + } > + function selectionAsString() > + { > + return "(" + nodeAsString(getSelection().anchorNode) > + + ", " + getSelection().anchorOffset > + + "), (" + nodeAsString(getSelection().focusNode) > + + ", " + getSelection().focusOffset + ")"; > + } > + function checkSelection(step, expected) > + { > + if (selectionAsString() !== expected) { > + document.getElementById("result").innerHTML = "FAIL: After step " + step + " selection was " + selectionAsString(); > + return true; > + } > + return false; > + }
Darin Adler
Comment 5 2009-03-20 16:41:20 PDT
(In reply to comment #4) > I think you need to check here that the parent is of the same editability as > the child like in positionForPointRespectingEditingBoundaries. I don't think I do in practice. Everything has to be anonymous. I think you’ll have some non-anonymous nodes anywhere you’d have an editability transition. By the way, what do you think we’d do when the parent’s editability differed from the child? > Maybe add the following test cases to test the rest of this? > -Editable element with generated content only at the beginning > -Editable element with only generated content inside We’d have to add tons more test cases to test all of these. I can try to make more, but I think this fix can go in before we add tests to cover every single one of the cases. > > + // Find non-anonymous content before. > > + renderer = child; > > + while ((renderer = renderer->previousInPreOrder())) { > > + if (renderer == parent) > > + break; > > + if (Node* node = renderer->node()) > > + return VisiblePosition(node, numeric_limits<int>::max(), DOWNSTREAM); > > + } > > + > > + // Use the parent itself unless it too is anonymous. > > + if (Node* node = parent->node()) > > + return VisiblePosition(node, 0, DOWNSTREAM); > > + > > + // Repeat at the next level up. > > + child = parent; > > + } > > Is this actually possible? If so, would be good to have a test case. Maybe this > even deserves an ASSERT(false)? Sure, I have no idea which of these pathological cases really exist and which don’t. Making test cases for these are extremely challenging. We can add more test cases after I land the initial patch. > Not a big deal, but > LayoutTests/editing/selection/resources/js-test-selection-shared.js already has > equivalents to the first three of these methods. Although it's DOM-based in > it's asserts rather than string-based and it assumes you pull in > js-test-pre.js. And these exact three function came from yet another test. I’d be happy to see lots of other high-quality tests and refactoring to include this test. But at the moment I’m satisfied having at least one test to demonstrate the need for this fix.
Ojan Vafai
Comment 6 2009-03-20 16:49:29 PDT
Yeah, it occurred to me afterwards that we probably can't cross an editability boundary here since we stop at the first non-anonymous node we see. Maybe it's worth adding a comment to that effect for the next guy who messes with this code?
Darin Adler
Comment 7 2009-03-20 16:55:22 PDT
(In reply to comment #6) > Yeah, it occurred to me afterwards that we probably can't cross an editability > boundary here since we stop at the first non-anonymous node we see. Maybe it's > worth adding a comment to that effect for the next guy who messes with this > code? Will do.
Darin Adler
Comment 8 2009-03-23 17:51:42 PDT
http://trac.webkit.org/changeset/41928
Simon Fraser (smfr)
Comment 9 2009-03-24 14:18:30 PDT
Do we know which commit originally caused this regression?
Darin Adler
Comment 10 2009-03-24 14:23:45 PDT
(In reply to comment #9) > Do we know which commit originally caused this regression? I don't know which one. The cause was presumably indirect. We had this latent bug for a long time and I think it was changes to higher levels of the editing code that made it start to matter.
Eric Seidel (no email)
Comment 11 2009-03-24 14:25:32 PDT
Yes http://trac.webkit.org/changeset/41191 was the commit which would have exposed this latent bug to these more common circumstances.
Note You need to log in before you can comment on or make changes to this bug.