74404 - Drag&Drop: While dragging over editable text, need to show drop insertion point with a special caret

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Simon Fraser [no longer active]]

While we are tracking drags of droppable content over a text widget or editor 
content, we should show a 'dummy caret' to give feedback on where the text will 
be dropped. This is a big usability issue for drag and drop.

Comment 1

[Simon Fraser [no longer active]]

Catfood for me.

Comment 2

[Mike Pinkerton (not reading bugmail)]

mee too! (i'd almost say dogfood)

Comment 3

[rubydoo123]

yeah, this one fry's my bacon

Comment 4

[rubydoo123]

move to 9.2

Comment 5

[Simon Fraser [no longer active]]

mike/tony; would either of you like to take this one?

Comment 6

[basic]

Changing platform

Comment 7

[Caspy7]

Maybe this comment will give this bug a bit more attention.

Wait.  Can you hear it?  In a wispy voice it screams "Fix me...I'm a huge 
usability issue...fix me..."

Comment 8

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 9

[kinmoz]

*** Bug 125542 has been marked as a duplicate of this bug. ***

Comment 10

[Simon Fraser [no longer active]]

Pre-sabbatical bug triage.

Comment 11

[Charles Manske]

This will probably involve adding another "selection type". MJudge points out
that the normal caret only listens to "normal selection."
The caret should be in a different style than the normal caret, e.g., dotted 
like we did in 4.x

Comment 12

[saari (gone)]

topembed+ for providing better feedback when dragging and dropping in editor

Comment 13

[David Bradley]

I just accidentally did a query for topembed+ in the status whiteboard and this
bug turned up. Shouldn't that be moved to keywords?

Comment 14

[Kathleen :Brade]

moving topembed+ from status whiteboard to keyword

Comment 15

[Simon Fraser [no longer active]]

Taking.

Comment 16

[Simon Fraser [no longer active]]

Attachment: First cut patch

This patch allows the caret to be created, and drawn by anyone. It exposes
nsICaret via the layout components (so I can create an instance of one), and
refactors the caret drawing code so that I could add an API call to just draw
the caret at a given node and offset.

nsTextEditorDragListener then makes a caret, and redraws it as you drag around.


This patch works pretty well, apart from crashing on window close sometimes,
probably because the nsTextEditorDragListener's mPresShell has died before we
try to clean up the caret. This is the old problem of the editor outliving the
pres shell.

Comment 17

[Simon Fraser [no longer active]]

Attachment: Final caret drag feedback patch

Notes on the patch:

nsICaret.h:
  Change C-style 'typedef enum' to enum.
  Add Terminate() method for cleanup.
  Add DrawAtPosition() method to draw at the specified node and offset.
  
nsCaret.h:
  Remove NS_CARET_CID (moved to nsLayoutCID.h)
  Detabbed, fixing alignment issues (diff is -b)
  Adding new methods, changing signatures.
  
nsCaret.cpp:
  Initialize mKeyboardRTL for BIDI.
  
  nsCaret::Init:
    Get the nsILookAndFeel from the pres context, rather than doing a
    CreateInstance. This should be much faster.
    There was a bug here that set mCaretTwipsWidth from the
    nsILookAndFeel, rather than setting mCaretPixelsWidth. This
    was masked by the fact that SetCaretWidth was always called before.
    Cleaned up code to do early returns on failure.
  
  Changed QI/AddRef/Release to use NS_IMPL_ISUPPORTS2 macro.
  
  nsCaret::Terminate:
    New method for cleanup -- unregister as selection listener, and
    kill timer.
    
  nsCaret::GetCaretVisible
    Code was doing bogus if (!domSelection) test.
    
  nsCaret::SetCaretWidth
  nsCaret::SetVisibilityDuringSelection
    Moved up from below.
    
 nsCaret::DrawAtPosition
    New method, uses refactored SetupDrawingFrameAndOffset()
    and GetCaretRectAndInvert() methods.
    
 nsCaret::SetupDrawingFrameAndOffset
    This was refactored to allow the node, offst and hint to be
    passed in (which you'll see in DrawCaret).
    
 nsCaret::DrawCaret
    Get the node and offset to call SetupDrawingFrameAndOffset() with.
    The guts of DrawCaret were moved into GetCaretRectAndInvert().
    
 nsCaret::GetCaretRectAndInvert
    New method called from DrawCaret and DrawAtPosition().

nsPresShell.cpp
  Fix SetCaretWidth() declaration to reference pixels, not twips.
  call nsCaret::Terminate() before freeing the caret.
  Changed this and other code to be able to deal with a null caret.
  I don't see why we need to always make a caret for every pres shell
  (though we still do). (Note diff -b doesn't show indentation).

nsLayoutCID.h
nsLayoutModule.cpp
  Allow a caret to be created outside of layout.
  
nsEditorEventListeners.h
  Give the nsTextEditorDragListener a caret member. Also pass
  in a pres shell to the ctor, which is needed to init the caret.
  
nsEditorEventListeners.cpp
  nsTextEditorDragListener draws and erases the caret for each 
  dragover event. On drop or dragexit, the caret is erased. I
  refactored some code in DragDrop into 'CanDrop', which is now
  also called from DragOver, so that the drag feedback we show
  will always match the drop state -- dragging over the selection,
  or dragging unreadable flavors, will not show the insertion caret.

nsPlaintextEditor.cpp
nsHTMLEditor.cpp
  Changned callers of NS_NewEditorDragListener to pass in a pres shell.

Comment 18

[Kathleen :Brade]

Comment on attachment 114467 [details] [diff] [review]
Final caret drag feedback patch

in nsICaret.h, fix the comment for DrawAtPosition:
... call EraseCaret() before after each call to...

In nsCaret.h, remove tab on SetCaretVisible line

Wrap SetupDrawingFrameAndOffset to < 80 columns.

Fix the comment for mCaretTwipsWidth:  "lazily"
Ideally keep the line under 80 cols
Perhaps just change the comment to be "This gets calculated lazily" since it
seems obvious to me that it's the width of the caret in twips (based on the
name)

Add a check for privateSelection in this area:
       privateSelection = do_QueryInterface(domSelection);
       privateSelection->AddSelectionListener(this);

Capitalize "this" and rewrap to <80 columns:
+  // this doesn't erase the caret if it's drawn. Should it? We might not have
a good

Fix "unregiser" on this line:
+  // unregiser ourselves as a selection listener

Wrap these lines to < 80 columns:
+PRBool nsCaret::SetupDrawingFrameAndOffset(nsIDOMNode* aNode, PRInt32 aOffset,
nsIFrameSelection::HINT aFrameHint)

+  nsresult rv = frameSelection->GetFrameForNodeOffset(contentNode, aOffset,
aFrameHint, &theFrame, &theFrameOffset);

Put spaces before the comment on this line:
+    privateSelection->GetInterlinePosition(&hintRight);//translate hint.

I think we need a check for a null caret after these lines:
http://lxr.mozilla.org/seamonkey/source/editor/libeditor/text/nsPlaintextEditor
.cpp#1978
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsSelection.cpp#3177
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsTextControlFram
e.cpp#627
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsTextControlFram
e.cpp#644
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsTextControlFram
e.cpp#666

In nsEditorEventListeners.cpp, remove the printfs or wrap in #ifdef DEBUG

Why comment out this line? Can it be removed?
+	 //mCaret->SetCaretVisible(PR_TRUE);   // make sure it's visible

You could change DragExit to be like DragOver:
  if (mCaret && mCaretDrawn)
  {
    mCaret->EraseCaret();
    mCaretDrawn = PR_FALSE;
  }

Fix all the comments that start with "//dont"

Fix the comment with "orginal" --> "original"

Comment 19

[kinmoz]

Comment on attachment 114467 [details] [diff] [review]
Final caret drag feedback patch

sr=[e-mail], just address brade's issues and the following:


==== The declaration for privateSelection precedes these lines. Do we want to
combine the declaration with the do_QI() call?


       privateSelection = do_QueryInterface(domSelection);
       privateSelection->AddSelectionListener(this);


==== nscoord can be negative also, should this be |aPixels <= 0| or |aPixels <
1|?


+NS_IMETHODIMP nsCaret::SetCaretWidth(nscoord aPixels)
+{
+  if (aPixels == 0)
+    return NS_ERROR_FAILURE;


==== Hard coding the hint might mean the drop caret will appear in the wrong
place when dragging at the start of a line that 

was wrapped. Perhaps we should put an XXX comment that notes that for now.


+  if (!SetupDrawingFrameAndOffset(aNode, aOffset,
nsIFrameSelection::HINTLEFT))	// bogus hint


==== This comment isn't true when mShowDuringSelection is true.

+
+    // start and end parent should be the same since we are collapsed
+    nsCOMPtr<nsIDOMNode>  focusNode;
+    domSelection->GetFocusNode(getter_AddRefs(focusNode));
+    if (!focusNode)
+      return;



==== Do we want to rename |pixels| to |aPixels| to match the other method
decls?


   NS_IMETHOD SetCaretEnabled(PRBool aInEnable);
-  NS_IMETHOD SetCaretWidth(PRInt16 twips);
+  NS_IMETHOD SetCaretWidth(PRInt16 pixels);
   NS_IMETHOD SetCaretReadOnly(PRBool aReadOnly);


==== I tend to dislike assignments in macros cause they can assign more than
once?


+  NS_IF_ADDREF(*outCaret = mCaret);


==== Remove your debug printfs or make them ifdef DEBUG_sfraser:


+  printf("nsTextEditorDragListener::DragEnter\n");

+  printf("nsTextEditorDragListener::DragOver\n");

+  printf("nsTextEditorDragListener::DragExit\n");

+  printf("nsTextEditorDragListener::DragDrop\n");

Comment 20

[Simon Fraser [no longer active]]

Checked in with kin's comments addressed.

Comment 21

[rubydoo123]

using the build from 2003031804 on win32, this now works.
1. opened composer, entered some text
2. selected some text from this bug report, selected copy and dragged to
composer window
3. when the caret was moved into the compsor window, the caret symbol changed
from the arrow/I-beam to an arrow with a bordered box.

Comment 22

[Roger M. Wilcox]

Beppe:  What you describe is the mouse cursor, not a caret.

Comment 23

[rubydoo123]

thanks for the correction, I will try to use the right words in the future

Comment 24

[kinmoz]

Just an FYI for anyone porting this patch to a branch older than mozilla1.4alpha
... you will also need the patch from 199133 which gets D&D working in embedding
builds and prevents some of the drop caret cruft people have been seeing.