Closed Bug 1105184 Opened 7 years ago Closed 7 years ago
Caret(text insertion point) does not display during text/image drag in contenteditable(CKEditor/Tny
[Tracking Requested - why for this release]: regression Steps To Reproduce: 0. Open non-e10s window (this is necessary until bug 936092 fixed) 1. Open any contenteditable (e.g. URL) 2. Select something 3. Attempt to drag and drop selected text Actual Results: Caret(text insertion point) does not display during drag. So, it is difficult to determine actual drop position. Expected Results: Caret(text insertion point) should display during drag.
Regression window(m-i) Good: https://hg.mozilla.org/integration/mozilla-inbound/rev/b8041e7ee525 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140815202314 Bad: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa1ea174e770 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140815214514 Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b8041e7ee525&tochange=fa1ea174e770 Regressed by: Bug 1048752
Bug 1102906 looks to be related although it is an older bug. From my understanding of this bug, I don't think this is a stop ship issue for 34. As such, I have marked 34 as wontfix. I have tracked for 35+.
Summary: Caret(text insertion point) does not display during test/image drag in contenteditable(CKEditor/TnyMCE/Thunderbird) → Caret(text insertion point) does not display during text/image drag in contenteditable(CKEditor/TnyMCE/Thunderbird)
nsEditorEventListener::DragOver used to call EraseCaret + DrawAtPosition which pretty much bypasses all nsCaret's internal checks for when to draw the caret. https://hg.mozilla.org/releases/mozilla-release/annotate/f05a36b5b90d/editor/libeditor/nsEditorEventListener.cpp#l698 Now we have to have IsVisible() true: https://hg.mozilla.org/releases/mozilla-release/annotate/62287d20bf35/layout/base/nsCaret.cpp#l245 which includes having a collapsed Selection, which is never true when dragging a selection from within the same shell. SetVisibilityDuringSelection(true) avoids that check. This patch seems to fix it, but needs more testing.
Assignee: nobody → mats
Comment on attachment 8529427 [details] [diff] [review] wip Review of attachment 8529427 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/nsEditorEventListener.cpp @@ +849,5 @@ > > + // This is to avoid the requirement that the Selection is Collapsed which > + // it can't be when dragging a selection in the same shell. > + // See nsCaret::IsVisible(). > + mCaret->SetVisibilityDuringSelection(true); Shouldn't we be resetting this to false at some point?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4) > Shouldn't we be resetting this to false at some point? I don't think it's needed since the mCaret here is a temporary caret just for the drag operation (and we always want it true for drag). It's created in DragEnter: http://hg.mozilla.org/mozilla-central/annotate/17de0f463944/editor/libeditor/nsEditorEventListener.cpp#l805 and then destroyed in DragExit or Drop (a page down from the above link). We can setup the caret with SetVisibilityDuringSelection(true) unconditionally - DragOver will call SetVisible(true/false) as appropriate and the VisibilityDuringSelection state doesn't really matter when SetVisible(false). https://tbpl.mozilla.org/?tree=Try&rev=b4d56ce986a4
Attachment #8530399 - Flags: review?(roc) → review+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Is this safe to uplift? We'd need to have a nomination for consideration before Mon Dec 22 to get into 35. Could also wontfix for 35 and ride from 36 if too risky.
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #8) > Is this safe to uplift? We'd need to have a nomination for consideration > before Mon Dec 22 to get into 35. Could also wontfix for 35 and ride from > 36 if too risky. Yes, I think this is a low-risk patch.
Unfortunately without a nomination, this missed the last viable beta for 35 - wontfixing - please nominate for aurora uplift to get this on 36.
Comment on attachment 8530399 [details] [diff] [review] fix Approval Request Comment [Feature/regressing bug #]:1048752 [User impact if declined]: missing text caret during DND [Describe test coverage new/current, TBPL]: on m-c since 2014-12-01 [Risks and why]: low risk [String/UUID change made/needed]:none
Attachment #8530399 - Flags: approval-mozilla-aurora?
Attachment #8530399 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.