Bug 8159 – REGRESSION: Clicking outside new text field focuses the field

• Component: HTML Editing
• Status: RESOLVED FIXED

In the attached test case, clicking at any vertical coordinate above the bottom of the text field or below the bottom of the yellow rectangle causes the field to focus. Clicking inside the yellow rectangle but below the text field unfocuses it. The horizontal "sensitive areas" are evident whenever there is no text to the left (right) of a text field. Clicking there will focus the field.

Created attachment 7486 [details]
Test case

[Darin Adler]

I believe this has the same cause as bug 8072.

[Darin Adler]

Assigning to Adele since she's working on min/max width, and I believe that change will probably fix this bug.

[Adele Peterson]

This wasn't fixed by my min/max change :(

[Darin Adler]

Dave said he thinks the issue is related to the hit testing in RenderBlock::positionForCoordinates.

[Darin Adler]

Created attachment 7606 [details]
another test case showing the same thing with contenteditable

The same thing happens with contenteditable (and this is not new) as demonstrated by this second test case.

[Darin Adler]

I solved the first part of this, fixing RenderBlock::positionForCoordinates so it creates a position after the input element (or content-editable div), but then it seems that VisiblePosition ends up putting the position inside anyway. So now I think we need to fix VisiblePosition.

[Darin Adler]

Created attachment 7700 [details]
patch that fixes the problem but causes other regressions

Here's what I have so far.

[Darin Adler]

Created attachment 7755 [details]
patch that solves the problem

I think this patch is going to do the trick. I'm going to talk to Justin about it on Monday. I tried versions of the patch that did less to remove deepEquivalent, but removing some but not all uses of deepEquivalent caused lots of problems.

[Justin Garcia]

Why the UPSTREAM affinity here?

+    if (y >= bottom || (isEditableRoot && (y >= top && x >= right))) {
+        if (isEditableRoot) {
+            if (Node* p = n->parent())
+                return VisiblePosition(p, n->nodeIndex() + 1, UPSTREAM);
+            return VisiblePosition(n, 0, DOWNSTREAM);
+        }

What if next is not in the original root editable element?

+        else {
+            Node* originalRootEditable = position.node()->rootEditableElement();
+            Node* prevRootEditable = prev.node()->rootEditableElement();
+            if (originalRootEditable == prevRootEditable)
+                m_deepPosition = prev;
+            else
+                m_deepPosition = next;
+        }

We'll have to talk about this change in person I think:

-        if (!insertionBlockIsRoot && fragment.isBlockFlow(refNode.get()) && isStartOfBlock(visibleInsertionPos))
-            insertNodeBeforeAndUpdateNodesInserted(refNode.get(), insertionBlock);
-        else if (!insertionBlockIsRoot && fragment.isBlockFlow(refNode.get()) && isEndOfBlock(visibleInsertionPos)) {
-            insertNodeAfterAndUpdateNodesInserted(refNode.get(), insertionBlock);
+        if (!insertionBlockIsRoot && fragment.isBlockFlow(refNode.get()) && isStartOfBlock(visibleInsertionPos)) {
+            Node* insertRefNode = insertionBlock;
+            if (insertionBlock->firstChild() && insertionBlock->firstChild()->isBlockFlow())
+                insertRefNode = insertionBlock->firstChild();
+            insertNodeBeforeAndUpdateNodesInserted(refNode.get(), insertRefNode);
+        } else if (!insertionBlockIsRoot && fragment.isBlockFlow(refNode.get()) && isEndOfBlock(visibleInsertionPos)) {
+            Node* insertRefNode = insertionBlock;
+            if (insertionBlock->lastChild() && insertionBlock->lastChild()->isBlockFlow())
+                insertRefNode = insertionBlock->lastChild();
+            insertNodeAfterAndUpdateNodesInserted(refNode.get(), insertRefNode);

Everything else looks good.

[Darin Adler]

The patch attached here affects 12 of the layout tests, but I'm pretty sure that the changes are all OK, except for this one:

    editing/unsupported-content/table-delete-001.html

The results from that test show a problem -- the table is not deleted -- which should be straightforward to fix.

But playing around with the test page shows difficulty selecting *within* the table, which is another serious problem that I should resolve before landing this.

[Darin Adler]

Created attachment 7890 [details]
latest version of patch that solves the problem

[Darin Adler]

Comment on attachment 7890 [details]
latest version of patch that solves the problem

Justin has reviewed all of this except for the changes in htmlediting.cpp.

[Darin Adler]

This latest version has a few major shortcomings. For one thing, it doesn't fix the original problem! I've made further progress and created some test cases and I'll attach those as soon as I can.

[Darin Adler]

Created attachment 7941 [details]
newer patch, still not done

[Darin Adler]

Created attachment 7959 [details]
patch, just about ready to go

[Justin Garcia]

+    // FIXME: No need for affinity parameter.

initDeepPosition doesn't use the affinity parameter but it needs to use it.  In the case where two candidates straddle a line wrap, we need to use the one that's at upstream() for an UPSTREAM affinity and the downstream() one otherwise.  Right now we always use the upstream() candidate.

+    // FIXME: Would read nicer if this was a function that returned a Position.

Yea.

+        table to qualify. While this is slightly sloppy, the old code worked because
+        of the "deep equivalent" technique. This change is needed to get the desired

I can't figure out how the old first/last in special element code relied on deepifying, but the new code is much better.

[Darin Adler]

Created attachment 8056 [details]
patch, including tests, detailed change log

[Justin Garcia]

Comment on attachment 8056 [details]
patch, including tests, detailed change log

+ (WebCore::RenderBlock::positionForCoordinates): also calls positionForCoordinates 
+ on children rather than calling positionForRenderer
The change is great but I don't think you included any layout tests for it.

These are incorrect results afaik because of 8350:
+        * editing/selection/iframe-expected.checksum:
+        * editing/selection/iframe-expected.png:
+        * editing/selection/inline-table-expected.checksum:
+        * editing/selection/inline-table-expected.png:
+        Test results now show spelling underlines again.
But maybe you should check them in anyway to remove the failures.  Whoever fixes 8350 can check in new fixed results.

r=me!

[Darin Adler]

(In reply to comment #19)
> The change is great but I don't think you included any layout tests for it.

Actually, the layout tests I added do test this. Without the change they won't work.

> These are incorrect results afaik because of 8350:
> +        * editing/selection/iframe-expected.checksum:
> +        * editing/selection/iframe-expected.png:
> +        * editing/selection/inline-table-expected.checksum:
> +        * editing/selection/inline-table-expected.png:
> +        Test results now show spelling underlines again.
> But maybe you should check them in anyway to remove the failures.  Whoever
> fixes 8350 can check in new fixed results.

Good point. I don't know if I should land them or not. I'll think it over.

[Darin Adler]

With the latest TOT, including Justin's last round of editing changes, and my patch applied, I crash inside the editing/pasteboard/paste-4039777-fix.html test.

=================
ASSERTION FAILED: selection.isCaretOrRange() (/Volumes/Home/darin/Safari/OpenSource/WebCore/editing/ReplaceSelectionCommand.cpp:516 virtual void WebCore::ReplaceSelectionCommand::doApply())
=================

#0  0x0200fd68 in WebCore::ReplaceSelectionCommand::doApply (this=0xf9102e0) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/ReplaceSelectionCommand.cpp:516
#1  0x01ffa4d4 in WebCore::EditCommand::apply (this=0xf9102e0) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/EditCommand.cpp:225
#2  0x01ffa628 in WebCore::EditCommandPtr::apply (this=0xbfffb124) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/EditCommand.cpp:79
#3  0x01fefc68 in WebCore::CompositeEditCommand::applyCommandToComposite (this=0x22d6c550, cmd=@0xbfffb124) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/CompositeEditCommand.cpp:104
#4  0x01ff27cc in WebCore::CompositeEditCommand::moveParagraph (this=0x22d6c550, startOfParagraphToMove=@0xbfffb1a0, endOfParagraphToMove=@0xbfffb1b8, destination=@0xbfffb1ac) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/CompositeEditCommand.cpp:731
#5  0x01ff4fbc in WebCore::DeleteSelectionCommand::mergeParagraphs (this=0x22d6c550) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/DeleteSelectionCommand.cpp:505
#6  0x01ff8268 in WebCore::DeleteSelectionCommand::doApply (this=0x22d6c550) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/DeleteSelectionCommand.cpp:635
#7  0x01ffa4d4 in WebCore::EditCommand::apply (this=0x22d6c550) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/EditCommand.cpp:225
#8  0x01ffa628 in WebCore::EditCommandPtr::apply (this=0xbfffb598) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/EditCommand.cpp:79
#9  0x01fefc68 in WebCore::CompositeEditCommand::applyCommandToComposite (this=0x22d67910, cmd=@0xbfffb598) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/CompositeEditCommand.cpp:104
#10 0x01ff0304 in WebCore::CompositeEditCommand::deleteSelection (this=0x22d67910, smartDelete=false, mergeBlocksAfterDelete=true) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/CompositeEditCommand.cpp:356
#11 0x0201017c in WebCore::ReplaceSelectionCommand::doApply (this=0x22d67910) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/ReplaceSelectionCommand.cpp:551
#12 0x01ffa4d4 in WebCore::EditCommand::apply (this=0x22d67910) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/EditCommand.cpp:225
#13 0x01ffa628 in WebCore::EditCommandPtr::apply (this=0xbfffbcc8) at /Volumes/Home/darin/Safari/OpenSource/WebCore/editing/EditCommand.cpp:79