Closed Bug 74404 Opened 24 years ago Closed 22 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.4alpha

People

(Reporter: sfraser_bugs, Assigned: sfraser_bugs)

References

Details

(Keywords: topembed+, Whiteboard: [caret]editorbase+ edt_x3)

Attachments

(1 file, 1 obsolete file)

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.
Catfood for me.
Keywords: nsCatFood
mee too! (i'd almost say dogfood)
yeah, this one fry's my bacon
Priority: -- → P3
Target Milestone: --- → mozilla0.9.1
move to 9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2
mike/tony; would either of you like to take this one?
Whiteboard: [caret]
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Target Milestone: mozilla0.9.3 → mozilla1.0
Changing platform
Hardware: All → Macintosh
OS: Mac System 8.5 → All
Hardware: Macintosh → All
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..."
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)
Target Milestone: mozilla1.0 → mozilla1.0.1
*** Bug 125542 has been marked as a duplicate of this bug. ***
Pre-sabbatical bug triage.
Target Milestone: mozilla1.0.1 → mozilla1.2alpha
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
Assignee: sfraser → mjudge
Keywords: nsbeta1
Summary: While dragging over editable text, need to show drop insertion point → Drag&Drop: While dragging over editable text, need to show drop insertion point with a special caret
Whiteboard: [caret] → [caret]editorbase
Target Milestone: mozilla1.2alpha → mozilla1.3beta
Keywords: nsbeta1nsbeta1+
Whiteboard: [caret]editorbase → [caret]editorbase+
Whiteboard: [caret]editorbase+ → [caret]editorbase+, topembed
Blocks: 182036
topembed+ for providing better feedback when dragging and dropping in editor
Whiteboard: [caret]editorbase+, topembed → [caret]editorbase+, topembed+
Target Milestone: mozilla1.3beta → mozilla1.4alpha
I just accidentally did a query for topembed+ in the status whiteboard and this bug turned up. Shouldn't that be moved to keywords?
moving topembed+ from status whiteboard to keyword
Keywords: topembed+
Whiteboard: [caret]editorbase+, topembed+ → [caret]editorbase+
Taking.
Assignee: mjudge → sfraser
Attached patch First cut patch (obsolete) — Splinter Review
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.
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.
Attachment #114067 - Attachment is obsolete: true
Attachment #114467 - Flags: superreview?(kin)
Attachment #114467 - Flags: review?(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"
Attachment #114467 - Flags: review?(brade) → review+
QA Contact: sujay → beppe
Whiteboard: [caret]editorbase+ → [caret]editorbase+ edt_x3
Comment on attachment 114467 [details] [diff] [review] Final caret drag feedback patch sr=kin@netscape.com, 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");
Attachment #114467 - Flags: superreview?(kin) → superreview+
Checked in with kin's comments addressed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Beppe: What you describe is the mouse cursor, not a caret.
thanks for the correction, I will try to use the right words in the future
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: