WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
8159
REGRESSION: Clicking outside new text field focuses the field
https://bugs.webkit.org/show_bug.cgi?id=8159
Summary
REGRESSION: Clicking outside new text field focuses the field
mitz
Reported
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.
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
mitz
Comment 1
2006-04-03 10:46:19 PDT
Created
attachment 7486
[details]
Test case
Darin Adler
Comment 2
2006-04-03 21:06:48 PDT
I believe this has the same cause as
bug 8072
.
Darin Adler
Comment 3
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.
Adele Peterson
Comment 4
2006-04-05 11:21:26 PDT
This wasn't fixed by my min/max change :(
Darin Adler
Comment 5
2006-04-08 17:06:45 PDT
Dave said he thinks the issue is related to the hit testing in RenderBlock::positionForCoordinates.
Darin Adler
Comment 6
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.
Darin Adler
Comment 7
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.
Darin Adler
Comment 8
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.
Darin Adler
Comment 9
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.
Justin Garcia
Comment 10
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.
Darin Adler
Comment 11
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.
Darin Adler
Comment 12
2006-04-21 18:33:07 PDT
Created
attachment 7890
[details]
latest version of patch that solves the problem
Darin Adler
Comment 13
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.
Darin Adler
Comment 14
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.
Darin Adler
Comment 15
2006-04-24 09:48:04 PDT
Created
attachment 7941
[details]
newer patch, still not done
Darin Adler
Comment 16
2006-04-25 09:15:31 PDT
Created
attachment 7959
[details]
patch, just about ready to go
Justin Garcia
Comment 17
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.
Darin Adler
Comment 18
2006-05-01 14:49:42 PDT
Created
attachment 8056
[details]
patch, including tests, detailed change log
Justin Garcia
Comment 19
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!
Darin Adler
Comment 20
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.
Darin Adler
Comment 21
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug