Caret(text insertion point) does not display during text/image drag in contenteditable(CKEditor/TnyMCE/Thunderbird)

RESOLVED FIXED in Firefox 36

Status

()

--
blocker
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: alice0775, Assigned: mats)

Tracking

({regression})

34 Branch
mozilla37
x86_64
All
regression
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox33 unaffected, firefox34- wontfix, firefox35+ wontfix, firefox36+ fixed, firefox37 fixed)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
[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.
(Reporter)

Comment 1

4 years ago
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
Blocks: 1048752
Flags: needinfo?(roc)
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+.
status-firefox34: affected → wontfix
tracking-firefox34: ? → -
tracking-firefox35: ? → +
tracking-firefox36: ? → +
(Reporter)

Updated

4 years ago
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)
(Assignee)

Comment 3

4 years ago
Created attachment 8529427 [details] [diff] [review]
wip

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
Flags: needinfo?(roc)
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?
(Assignee)

Comment 5

4 years ago
Created attachment 8530399 [details] [diff] [review]
fix

(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 #8529427 - Attachment is obsolete: true
Attachment #8530399 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/05ef711f1e13
Status: NEW → RESOLVED
Last Resolved: 4 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.
Flags: needinfo?(mats)
(Assignee)

Comment 9

4 years ago
(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.
Flags: needinfo?(mats)
Unfortunately without a nomination, this missed the last viable beta for 35 - wontfixing - please nominate for aurora uplift to get this on 36.
status-firefox35: affected → wontfix
status-firefox37: --- → fixed
Flags: needinfo?(mats)
(Assignee)

Comment 11

4 years ago
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
Flags: needinfo?(mats)
Attachment #8530399 - Flags: approval-mozilla-aurora?
Attachment #8530399 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 12

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/b6b89746c58b
status-firefox36: affected → fixed

Updated

4 years ago
Keywords: dev-doc-needed
QA Whiteboard: [good first verify]

Updated

3 years ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.