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.
Created attachment 28802 [details] first cut at a test, made by Eric
rdar://problem/6696992
Created attachment 28805 [details] patch
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; > + }
(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.
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?
(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.
http://trac.webkit.org/changeset/41928
Do we know which commit originally caused this regression?
(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.
Yes http://trac.webkit.org/changeset/41191 was the commit which would have exposed this latent bug to these more common circumstances.