Summary: | REGRESSION: Clicking outside new text field focuses the field | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||||||||||||||||
Component: | HTML Editing | Assignee: | Darin Adler <darin> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | adele, justin.garcia | ||||||||||||||||||
Priority: | P1 | Keywords: | Regression | ||||||||||||||||||
Version: | 420+ | ||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||||||
Attachments: |
|
Description
mitz
2006-04-03 10:45:56 PDT
Created attachment 7486 [details]
Test case
Assigning to Adele since she's working on min/max width, and I believe that change will probably fix this bug. This wasn't fixed by my min/max change :( Dave said he thinks the issue is related to the hit testing in RenderBlock::positionForCoordinates. 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.
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. Created attachment 7700 [details]
patch that fixes the problem but causes other regressions
Here's what I have so far.
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.
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. 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. Created attachment 7890 [details]
latest version of patch that solves the problem
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.
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. Created attachment 7941 [details]
newer patch, still not done
Created attachment 7959 [details]
patch, just about ready to go
+ // 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. Created attachment 8056 [details]
patch, including tests, detailed change log
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!
(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. 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 |