do let me know if this is a dup. noticed this on linux, using 2000-01-11-10 comm bits. 1. in a browser window, go to a text field (eg, the Summary field for this bug report) --move (but don't click yet) the mouse pointer to somewhere in the middle of the text (eg, hover over the word "clicking" in this Summary). 2. double-click the left mouse button to select. result: after the 1st click, all of the Summary text is selected, but after the 2nd click has completed, only the text from the beginning of the field to the middle of the word "clicking" is selected. this occurs even if i double-click quickly. expected: after the 1st click, nothing should be selected. after the 2nd click, the entire text field contents should be selected. Eli mentioned that this bug has always been around --i've only noticed it within this week's builds. perhaps something was recently changed making this even more sensitive?

[thanks! actually, double-clicking is only supposed to select a word. Triple- clicking is normally the gesture used to select a line, although we're not supporting it.]

Doh. This is messed up. Will comment further after Mike fixes this. mjudge, are we actually supporting triple-click? I thought you yanked that.

triple-clicking *should* work; however, it won't work on Windows until another bug gets fixed (rods' bug?)

setting this to m14

moving to m15

This only happens to me if I move the cursor (even slightly) during the second click. I think the best solution for this problem is to impliment the request for enhancement in bug #16203.

P4

moving to M17

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

dup of either 16203, or 21090. dbl click should select in word by word mode. or double clicking errantly selecting from click point to begining of line. *** This bug has been marked as a duplicate of 16203 ***

this is actually not a duplicate of #16203; it's a better match to #21090 This bug has to do with the mouse moving while making a double-click (mouse comes up). Bug #16203 has to do with click-and-a-half (if you will) which is word-by-word selection (mouse is still down when you make selection).

Yes, it seems that bug 16203 is not exactly the same as this bug. However 21090 does seem a duplicate of this bug. Lets check...

bug 21090 is indeed a dulplicate of this bug

The day after resolving this as duplicate of bug 16203, brade wrote in comment 11: "this is actually not a duplicate of #16203" None the less, someone verified it as duplicate a week later. Comment 12 also mentions that it doesn't seem to be a duplicate, and it doesn't look like a duplicate to me either. Reopening for clarification.

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

Similar: bug 97822, "Click on URL bar selects only first half of URL (if mouse moves *at all* during click)".

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

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

target milestone currently set to M17... future perhaps? Platform -> PC / All ? OS -> All.

Nominating for editorbase -- this is much more basic and common than bug 16203 (doubleclick followed by drag) which was just marked editorbase+, and everyone in the last editorbase meeting seemed to agree that it was important enough to warrant it.

ugh, sorry, its late and I'm not fully groking the bug here. Akkana, can you clarify what the issue is?

When you double-click in the urlbar, you should get the whole word (which usually means the whole url, unless there's a space in it) selected. What actually happens, most of the time, is that it selects from the beginning of the url to the mouse position, leaving the right half of the url unselected. This makes it very hard to paste the current url into documents like mail messages or irc, since it takes several tries to get the url selected.

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

this will stop drag selecting after a doubleclick until the user drags out of the currently selected text.

Comment on attachment 93773 [details] [diff] [review] fix for double click problem This is much better! It will really help usability. In nsSelection::HandleDrag: >+ PRInt8 index = GetIndexFromSelectionType(nsISelectionController::SELECTION_NORMAL); It seems a shame to have to do this for every drag event, since the index never changes, but I guess it's not a big deal (it's certainly better than just putting a magic number like "0" here). Inside the loop inside of nsSelection::HandleDrag: >+ nsCOMPtr<nsIDOMNode> domNode; >+ domNode = do_QueryInterface(newContent); It might be better to put the QI outside the loop, since it's not changing and performance testing has shown we generally spend too much time inside QI. It won't matter much in real life because we currently never have more than one range per selection, but someday we might. I was confused about why so much code was devoted to figuring out whether the drag event was inside the current selection, but Mike demonstrated that this is needed so you can down-up-down then drag out of the word and the selection will extend properly. Indeed, it behaves nicely now. It might help to add a slightly longer comment than the one that's there (partly I was confused because I was thinking "drag" as in "drag-n-drop" rather than "drag" as in "drag to extend the current selection"). r=akkana if you move the QI outside the loop (or explain why I'm wrong in thinking it doesn't change). Use your own judgement on the other two issues.

get index from selection type only will happen when initially dragging after a double click and you are dragging around the selected word. I will move the QI outside of the loop.

full patch adds better comment i think and makes the QI for domnode from content outside a for loop.

Comment on attachment 94209 [details] [diff] [review] fix for comment and QI outside of loop. transferring r= from before

updating qa_contact

Comment on attachment 94209 [details] [diff] [review] fix for comment and QI outside of loop. ***WARNING***: This attachment omits the nsIFrameSelection changes that were in the previous attachment. You need those. Don't forget to check them in. :-) >Index: y:/mozilla/layout/html/base/src/nsFrame.cpp >@@ -1302,21 +1304,22 @@ > PRInt32 contentOffsetEnd = 0; > nsCOMPtr<nsIContent> newContent; > PRBool beginContent = PR_FALSE; > rv = GetContentAndOffsetsFromPoint(aPresContext, > aEvent->point, > getter_AddRefs(newContent), > startPos, > contentOffsetEnd, > beginContent); > if (NS_FAILED(rv)) return rv; >- >+ >+ Seems like an odd whitespace change, particularly the spaces on the blank line (you did the same thing below), but it is your code, so if that's what you really want, it's fine with me. There are a bunch of other similar changes in the patch. >Index: y:/mozilla/layout/html/forms/src/nsTextControlFrame.cpp >+ return NS_ERROR_FAILURE; >+} >+ >+NS_IMETHODIMP nsTextInputSelectionImpl::SetMouseDoubleDown(PRBool aDoubleDown) >+{ >+ if(mFrameSelection) >+ return mFrameSelection->SetMouseDoubleDown(aDoubleDown); >+ return NS_ERROR_FAILURE; >+} >+ >+NS_IMETHODIMP nsTextInputSelectionImpl::GetMouseDoubleDown(PRBool *aDoubleDown) >+{ >+ if(mFrameSelection) >+ return mFrameSelection->GetMouseDoubleDown(aDoubleDown); > return NS_ERROR_FAILURE; > } Two general comments on this, neither of which require fixing for this patch, although you can if you want: 1) It usually makes more sense to me to do something like if (!mFrameSelection) return NS_ERROR_FAILURE; return mFrameSelection->...; since that's more in line both with the pattern that's used in longer functions and with the idea that nsresult values are like exceptions. 2) There are null checks for |mFrameSelection| in a lot of places in |nsTextInputSelectionImpl|, but there aren't in many others. It seems it would be much cleaner if you could remove all the null checks and replace them with a single null check (with an NS_ERROR_OUT_OF_MEMORY return), of the |frameSel| variable that's created by an |CreateInstance| (which really should be |do_CreateInstance|) right before the one place you call |new nsTextInputSelectionImpl|, plus an NS_PRECONDITION in the constructor and perhaps a comment at the member declaration to document that it's required to be non-null. >Index: y:/mozilla/content/base/src/nsSelection.cpp > nsIFocusTracker *mTracker; > PRBool mMouseDownState; //for drag purposes >+ PRBool mMouseDoubleDownState; //has the doubleclick down happened Make |mMouseDownState| and |mMouseDoubleDownState| both |PRPackedBool| and you'll save two words (since alignment will allow the two PRPackedBool, which are the same as PRUint8, to fit together with mDisplaySelection in a single word). (There don't seem to be any places where they're passed to a function expecting a |PRBool*| out parameter. And even if there were, you could use a temporary.) > PRInt16 mDisplaySelection; //for visual display purposes. > PRInt32 mDesiredX; >+ //Is the desired content and offset currently in selection? >+ //if the double click flag is set then dont continue selection if the desired content and offset >+ //are currently inside a selection. >+ //this will stop double click then mouse-drag from undo-ing the desired selecting of a word. >+ if (mMouseDoubleDownState) >+ { >+ nsFrameState frameState; >+ newFrame->GetFrameState(&frameState); >+ PRBool isSelected = (frameState & NS_FRAME_SELECTED_CONTENT) == NS_FRAME_SELECTED_CONTENT; >+ PRBool insideSelection(PR_FALSE); >+ if (isSelected) >+ { It seems like it would be slightly cleaner to remove the |isSelected| variable and make this just if (frameState & NS_FRAME_SELECTED_CONTENT) (Hopefully compilers would optimize the extra ==, but they might not, and all you really need to know is whether it's nonzero -- you don't need to convert any nonzero value to 1 and then test whether 1 is nonzero.) >+ PRBool isCollapsed; >+ PRInt8 index = GetIndexFromSelectionType(nsISelectionController::SELECTION_NORMAL); >+ mDomSelections[index]->GetIsCollapsed(&isCollapsed); >+ if (!isCollapsed) >+ { >+ PRInt32 rangeCount; >+ result = mDomSelections[index]->GetRangeCount(&rangeCount); >+ if (NS_FAILED(result)) return result; Probably better (for consistency, and for being able to set breakpoints in the debugger), to put the |return result| on a new line. >+ >+ nsCOMPtr<nsIDOMNode> domNode; >+ domNode = do_QueryInterface(newContent); >+ if (domNode) >+ { >+ for (PRInt32 i = 0; i < rangeCount; i++) >+ { >+ nsCOMPtr<nsIDOMRange> range; >+ >+ result = mDomSelections[index]->GetRangeAt(i, getter_AddRefs(range)); >+ if (NS_FAILED(result) || !range) >+ continue;//dont bail yet, iterate through them all >+ >+ nsCOMPtr<nsIDOMNSRange> nsrange(do_QueryInterface(range)); >+ if (NS_FAILED(result) || !nsrange) >+ continue;//dont bail yet, iterate through them all You didn't change |result| since the last time you checked it -- no need to check it twice. >+ >+ result = nsrange->IsPointInRange(domNode, startPos, &insideSelection); >+ >+ // Done when we find a range that we are in >+ if (insideSelection) >+ break; >+ } >+ } >+ } >+ } >+ if (!insideSelection) >+ mMouseDoubleDownState = PR_FALSE; >+ else >+ return NS_OK; //dragging in selection aborted >+ } It would be good to put an extra newline right here so that it's clearer that the braces below are just for scoping an |nsCOMPtr| in a block and not associated with an |else|, as they might *appear*. > // do we have CSS that changes selection behaviour? > { > PRBool changeSelection; > nsCOMPtr<nsIContent> selectContent; > PRInt32 newStart, newEnd; >+NS_IMETHODIMP >+nsSelection::GetMouseDoubleDown(PRBool *aDoubleDown) >+{ >+ if (!aDoubleDown) >+ return NS_ERROR_NULL_POINTER; >+ *aDoubleDown = mMouseDoubleDownState; > return NS_OK; > } I don't think it's necessary to null-check |aDoubleDown|. null-checking out parameters that are equivalent to IDL result types isn't a requirement of scriptability, and anybody who passes null is just silly. But if you want to null-check, use NS_ENSURE_ARG_POINTER(aDoubleDown), so at least someone gets an assertion if they're silly enough to pass null. sr=dbaron

What's the status of this? It's been 9 days since dbaron sr'ed it ...

fixed. all comments executed I think.

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

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

yep, this works with recent trunk bits.