From Bugzilla Helper: User-Agent: Mozilla/5.0 (X11; U; SunOS 5.7 sun4u; en-US; m18) Gecko/20001012 BuildID: 2000101221 When I double-click a word, then drag to the left to extend the selection, Mozilla does not select the characters I expect it to select. Reproducible: Always Steps to Reproduce: Double-click-and-hold to select a word in the body of a document. Drag to the left to extend the selection. Actual Results: The selection is extended to the left, but the word that was initially selected is not included in the selection. Expected Results: The selection should be extended to the left, but the word initially selected should be included in the selection. The problem occurs everywhere I've tried it, including input elements and dialog boxes.

I can confirm this bug; I see this behavior on Macintosh as well. Setting to future. I'm guessing if #16203 is fixed then we'll see the correct behavior for this bug as well.

Can we raise the priority on this? It's probably about 1/2 hour of work to fix this and I think little details like this contribute to people's perception of Mozilla. And for me in particular, every time I click in a search field on a web page to edit a query I end up doing it twice because I'm so used to "correct" behavior of double clicking. In particular, the "anchor" for the selection should be the beginning of the selection if the user drags to the right, but it should be the END of the selection if the user drags to the left. I don't think this really depends on 16203 being fixed. It will just probably be fixed at the same time if someone fixes 16203. Can we remove the dependency too?

I saw this on OpenVMS build 20010114, just thought I'd mention it.

changing selection qa to tpreston.

This is still a problem on RC2, Openvms 20020513

seek retriage/milestone setting

editorbase-. Usage does not seem to be common enough to warrant editorbase+. but important to fix, nsbeta1+.

Note that analogously, extending selection upwards after triple-clicking also loses selection.

EDITORBASE+ topembed+ normalization

working on this now. I am using a "maintained selection" in nsSelection.cpp to track the selection from a double click or a one stroke extended selection (clicking on an image for example). Then any extending of selection at that point will do some cute code to make sure that the old selection remains. single clicking somewhere else will remove the maintained selection.

fixes the problem. maintaints the selection after double clicks. there is still an issue with image selection and coninuing previously on the selection, but i get 100 asserts when clicking on images now so i cant fix this yet. thats another bug anyway.

this fixes the image issue. i thought about it and realized what was happening. i moved the adjustment for the maintain selection into its own helper method to be called from HandleClick and HandleDrag (handle click for shift clicking.)

first patch i will remove

Comment on attachment 118787 [details] [diff] [review] better patch. handles images from \mozilla\ Any word on the case you mentioned you were testing where you double click a word, then use the arrow keys to move the caret and then shift click somewhere else? Just a few nits ... ==== Add and extra "g" to the spelling of "draging" and capitalize the 'd' to be consistent. + /** MaintainSelection will track the current selection as being "sticky". draging or extending ==== I see this same chunk of code in nsFrame.cpp and nsTextFrame.cpp. Should we have a utility method in nsFrame that does this and is called from nsTextFrame.cpp so it's in one place? + + if (NS_SUCCEEDED(rv)) + { + nsCOMPtr<nsIFrameSelection> frameselection; + nsCOMPtr<nsISelectionController> selCon; + nsCOMPtr<nsIPresShell> shell; + if (NS_FAILED(aPresContext->GetShell(getter_AddRefs(shell)))) + return rv; //return old result we dont care if this fails + + if (NS_FAILED(GetSelectionController(aPresContext, getter_AddRefs(selCon))) || !selCon) + return rv; + frameselection = do_QueryInterface(selCon); //this MAY implement + if (!frameselection) + if (NS_FAILED(shell->GetFrameSelection(getter_AddRefs(frameselection))) || !frameselection) + return rv; + frameselection->MaintainSelection(); + } + + return rv; + ==== There are several places that set mMaintainRange to zero, use nsnull. I know it's the same thing but it makes it more apparent that it's a pointer. + mMaintainRange = 0; ==== Should probably check to make sure mMaintainRange is not null before dereferencing it. + NS_NewRange(getter_AddRefs(mMaintainRange)); + + mMaintainRange->SetStart(startNode, startOffset); + mMaintainRange->SetEnd(endNode, endOffset); ==== Capitalize the start of "if" and "this" since they start new sentences, just to be consistent with commenting style used in the file. + //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. ==== Remove |result| ... it never gets initialized or set before it is checked so I'm assuming it isn't needed at all. + if (mMaintainRange) + { + nsresult result; ... + if (domNode) + { ... + nsCOMPtr<nsIDOMNSRange> nsrange(do_QueryInterface(mMaintainRange)); + if (NS_SUCCEEDED(result) && nsrange) ==== Combine this into one line using |nsCOMPtr<nsIDOMNode> domNode(do_QueryInterface(aContent))| or |nsCOMPtr<nsIDOMNode> domNode = do_QueryInterface(aContent)|: + nsCOMPtr<nsIDOMNode> domNode; + domNode = do_QueryInterface(aContent); ==== Remove |isCollapsed|, it isn't used anywhere: + PRInt8 index = GetIndexFromSelectionType(nsISelectionController::SELECTION_NORMAL); + PRBool isCollapsed; ==== Why not just do a |return insideSelection;| instead of: + if (insideSelection) + return PR_TRUE; //dragging in selection aborted + } + return PR_FALSE; ==== Can we combine these 2 |if|s into one with |if (aContinueSelection) && Adjust*())|? + if (aContinueSelection) + if (AdjustForMaintainedSelection(aNewFocus, aContentOffset)) ==== Same here: + nsFrameState frameState; + newFrame->GetFrameState(&frameState); + if ((frameState & NS_FRAME_SELECTED_CONTENT)) { ... + if (AdjustForMaintainedSelection(newContent, startPos)) + return NS_OK; }

I have a better patch that addresses all these issues. i will post it tomorrow. having issues with cvs today for some reason

what is the latest on the patch for this one?

marking for next milestone

unsetting target milestone

-->brade I've taken the latest patch posted here and cleaned it up somewhat (hand merging as necessary). I still need to read over Kin's feedback and do that as well as do some testing to see what problems this patch does/doesn't have.

Comment on attachment 127771 [details] [diff] [review] clean up previous patch and make it apply to trunk carryforward r=danm The only "issue" I see with this bug doesn't seem to be a regression from this bug but a current behavior. It should probably be covered in a new bug (if it doesn't already exist). The steps are: 1) open editor 2) type this text: test test one two three four five six 3) double-click three and drag to two (dblClick-drag) 4) shift click to five observation: only "three" is selected seeking sr= to land this patch (which is on hardware that is about to disappear)

Comment on attachment 127771 [details] [diff] [review] clean up previous patch and make it apply to trunk >+nsSelection::MaintainSelection() >+ if (!mMaintainRange) return NS_ERROR_OUT_OF_MEMORY; This should probably be on two lines, for consistency. > nsSelection::HandleDrag(nsIPresContext *aPresContext, nsIFrame *aFrame, nsPoint& aPoint) >+ nsFrameState frameState; >+ newFrame->GetFrameState(&frameState); >+ if ((frameState & NS_FRAME_SELECTED_CONTENT) GetFrameState is deprecated; this should just be: if ((newFrame->GetStateBits() & NS_FRAME_SELECTED_CONTENT) >+ && AdjustForMaintainedSelection(newContent, startPos)) >+ { >+ return NS_OK; > } Also, Mozilla style is generally to put the && at the end of the previous line, and, if it fits with th surrounding code, you probably don't need the {} around the return. >Index: layout/base/public/nsIFrameSelection.h > NS_IMETHOD GetMouseDoubleDown(PRBool *aDoubleDown)=0; > >+ /** >+ * MaintainSelection will track the current selection as being "sticky". >+ * Dragging or extending selection will never allow for a subset >+ * (or the whole) of the maintained selection to become unselected. >+ * Primary use: double click selecting then dragging on second click >+ */ >+ NS_IMETHOD MaintainSelection()=0; > #ifdef IBMBIDI A blank line before the |#ifdef IBMBIDI| would be good. >Index: layout/html/base/src/nsFrame.cpp >+ // maintain selection >+ nsCOMPtr<nsISelectionController> selCon; >+ if (NS_FAILED(GetSelectionController(aPresContext, getter_AddRefs(selCon))) >+ || !selCon) >+ return NS_OK; Better to put the || at the end of the previous line. >+ >+ nsCOMPtr<nsIFrameSelection> frameselection = do_QueryInterface(selCon); >+ if (!frameselection) >+ { >+ nsCOMPtr<nsIPresShell> shell; >+ if (NS_FAILED(aPresContext->GetShell(getter_AddRefs(shell)))) >+ return NS_OK; // return NS_OK we don't care if this fails >+ >+ if (NS_FAILED(shell->GetFrameSelection(getter_AddRefs(frameselection))) >+ || !frameselection) >+ return NS_OK; This could just be: rv = aPresContext->GetPresShell()->GetFrameSelection(getter_AddRefs(frameselection)) ; if (NS_FAILED(rv) || !frameselection) return NS_OK; >+ } >+ >+ return frameselection->MaintainSelection(); > } > >Index: layout/html/base/src/nsTextFrame.cpp >+ // Maintain Selection >+ nsCOMPtr<nsISelectionController> selCon; >+ if (NS_FAILED(GetSelectionController(aPresContext, getter_AddRefs(selCon))) >+ || !selCon) >+ return NS_OK; >+ >+ nsCOMPtr<nsIFrameSelection> frameselection = do_QueryInterface(selCon); //this MAY implement >+ if (!frameselection) >+ { >+ nsCOMPtr<nsIPresShell> shell; >+ if (NS_FAILED(aPresContext->GetShell(getter_AddRefs(shell)))) >+ return NS_OK; //return NS_OK; we don't care if this fails >+ >+ if (NS_FAILED(shell->GetFrameSelection(getter_AddRefs(frameselection))) >+ || !frameselection) >+ return NS_OK; >+ } Same two comments here. (Do we really need this much copy-and-paste?) If you also double-check that all of kin's comments are addressed, sr=dbaron.

Comment on attachment 128174 [details] [diff] [review] close to final patch (cut and pasted together so may not apply cleanly) oops; ignore the changes in nsHTMLContainerFrame.cpp that isn't part of this patch

checked in