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)
Core
DOM: Editor
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)
43.45 KB,
patch
|
Brade
:
review+
kinmoz
:
superreview+
|
Details | Diff | Splinter Review |
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 2•24 years ago
|
||
mee too! (i'd almost say dogfood)
Comment 3•24 years ago
|
||
yeah, this one fry's my bacon
Priority: -- → P3
Target Milestone: --- → mozilla0.9.1
Assignee | ||
Comment 5•24 years ago
|
||
mike/tony; would either of you like to take this one?
Updated•24 years ago
|
Whiteboard: [caret]
Updated•24 years ago
|
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Updated•24 years ago
|
Target Milestone: mozilla0.9.3 → mozilla1.0
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•23 years ago
|
||
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. ***
Assignee | ||
Comment 10•23 years ago
|
||
Pre-sabbatical bug triage.
Target Milestone: mozilla1.0.1 → mozilla1.2alpha
Comment 11•22 years ago
|
||
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
Updated•22 years ago
|
Comment 12•22 years ago
|
||
topembed+ for providing better feedback when dragging and dropping in editor
Whiteboard: [caret]editorbase+, topembed → [caret]editorbase+, topembed+
Target Milestone: mozilla1.3beta → mozilla1.4alpha
Comment 13•22 years ago
|
||
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•22 years ago
|
||
moving topembed+ from status whiteboard to keyword
Keywords: topembed+
Whiteboard: [caret]editorbase+, topembed+ → [caret]editorbase+
Assignee | ||
Comment 16•22 years ago
|
||
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.
Assignee | ||
Comment 17•22 years ago
|
||
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
Assignee | ||
Updated•22 years ago
|
Attachment #114467 -
Flags: superreview?(kin)
Attachment #114467 -
Flags: review?(brade)
Comment 18•22 years ago
|
||
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+
Updated•22 years ago
|
QA Contact: sujay → beppe
Updated•22 years ago
|
Whiteboard: [caret]editorbase+ → [caret]editorbase+ edt_x3
Comment 19•22 years ago
|
||
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+
Assignee | ||
Comment 20•22 years ago
|
||
Checked in with kin's comments addressed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 21•22 years ago
|
||
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
Comment 22•22 years ago
|
||
Beppe: What you describe is the mouse cursor, not a caret.
Comment 23•22 years ago
|
||
thanks for the correction, I will try to use the right words in the future
Comment 24•22 years ago
|
||
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.
Description
•