Bug 8159 - REGRESSION: Clicking outside new text field focuses the field
Summary: REGRESSION: Clicking outside new text field focuses the field
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Darin Adler
URL:
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2006-04-03 10:45 PDT by mitz
Modified: 2006-05-03 13:03 PDT (History)
2 users (show)

See Also:


Attachments
Test case (114 bytes, text/html)
2006-04-03 10:46 PDT, mitz
no flags Details
another test case showing the same thing with contenteditable (193 bytes, text/html)
2006-04-09 16:54 PDT, Darin Adler
no flags Details
patch that fixes the problem but causes other regressions (5.83 KB, patch)
2006-04-14 08:57 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch that solves the problem (15.16 KB, patch)
2006-04-16 22:15 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
latest version of patch that solves the problem (23.44 KB, patch)
2006-04-21 18:33 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
newer patch, still not done (58.23 KB, patch)
2006-04-24 09:48 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch, just about ready to go (78.42 KB, patch)
2006-04-25 09:15 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch, including tests, detailed change log (112.67 KB, patch)
2006-05-01 14:49 PDT, Darin Adler
justin.garcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2006-04-03 10:45:56 PDT
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.
Comment 1 mitz 2006-04-03 10:46:19 PDT
Created attachment 7486 [details]
Test case
Comment 2 Darin Adler 2006-04-03 21:06:48 PDT
I believe this has the same cause as bug 8072.
Comment 3 Darin Adler 2006-04-03 21:12:18 PDT
Assigning to Adele since she's working on min/max width, and I believe that change will probably fix this bug.
Comment 4 Adele Peterson 2006-04-05 11:21:26 PDT
This wasn't fixed by my min/max change :(
Comment 5 Darin Adler 2006-04-08 17:06:45 PDT
Dave said he thinks the issue is related to the hit testing in RenderBlock::positionForCoordinates.
Comment 6 Darin Adler 2006-04-09 16:54:01 PDT
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.
Comment 7 Darin Adler 2006-04-13 02:34:27 PDT
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.
Comment 8 Darin Adler 2006-04-14 08:57:15 PDT
Created attachment 7700 [details]
patch that fixes the problem but causes other regressions

Here's what I have so far.
Comment 9 Darin Adler 2006-04-16 22:15:31 PDT
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.
Comment 10 Justin Garcia 2006-04-17 13:35:00 PDT
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.
Comment 11 Darin Adler 2006-04-20 15:11:32 PDT
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.
Comment 12 Darin Adler 2006-04-21 18:33:07 PDT
Created attachment 7890 [details]
latest version of patch that solves the problem
Comment 13 Darin Adler 2006-04-21 18:34:49 PDT
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.
Comment 14 Darin Adler 2006-04-23 20:28:03 PDT
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.
Comment 15 Darin Adler 2006-04-24 09:48:04 PDT
Created attachment 7941 [details]
newer patch, still not done
Comment 16 Darin Adler 2006-04-25 09:15:31 PDT
Created attachment 7959 [details]
patch, just about ready to go
Comment 17 Justin Garcia 2006-04-25 14:03:04 PDT
+    // 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.
Comment 18 Darin Adler 2006-05-01 14:49:42 PDT
Created attachment 8056 [details]
patch, including tests, detailed change log
Comment 19 Justin Garcia 2006-05-03 01:17:05 PDT
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!
Comment 20 Darin Adler 2006-05-03 09:20:07 PDT
(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.
Comment 21 Darin Adler 2006-05-03 11:52:58 PDT
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