Extending text selection to the left after double-clicking loses selection

RESOLVED FIXED

Status

()

Core
Selection
P2
normal
RESOLVED FIXED
18 years ago
15 years ago

People

(Reporter: davby, Assigned: Kathleen Brade)

Tracking

({topembed+})

Trunk
topembed+
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: editorbase+,edt_x3,edt_c3,edt_b3)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

18 years ago
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.
(Assignee)

Comment 1

18 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Solaris → All
Hardware: Sun → All
Target Milestone: --- → Future

Updated

17 years ago
Depends on: 16203

Comment 2

17 years ago
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?

Comment 3

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

Comment 4

16 years ago
changing selection qa to tpreston.
QA Contact: blaker → tpreston

Comment 5

16 years ago
This is still a problem on RC2, Openvms 20020513
(Assignee)

Comment 6

15 years ago
seek retriage/milestone setting
Blocks: 183112
Keywords: nsbeta1
Priority: P3 → P2
Summary: Extending text selection to the left after double-clicking behaves incorrectly → Extending text selection to the left after double-clicking loses selection
Whiteboard: editorbase
Target Milestone: Future → ---
editorbase-. Usage does not seem to be common enough to warrant editorbase+. but
important to fix, nsbeta1+.
Keywords: nsbeta1 → nsbeta1+
Whiteboard: editorbase → editorbase-

Comment 8

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

Updated

15 years ago
Whiteboard: editorbase- → editorbase+
QA Contact: tpreston → sairuh

Comment 9

15 years ago
EDITORBASE+ topembed+ normalization
Keywords: topembed+

Comment 10

15 years ago
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.
Status: NEW → ASSIGNED

Updated

15 years ago
Whiteboard: editorbase+ → editorbase+,edt_x3,edt_c3,edt_b3

Comment 11

15 years ago
Created attachment 118692 [details] [diff] [review]
patch for content and layout from \mozilla

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.

Comment 12

15 years ago
Created attachment 118787 [details] [diff] [review]
better patch. handles images from \mozilla\

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.)
Attachment #118692 - Attachment is obsolete: true

Updated

15 years ago
Blocks: 101454

Updated

15 years ago
Attachment #118692 - Attachment is obsolete: false
Attachment #118692 - Flags: review+

Updated

15 years ago
Attachment #118692 - Flags: review+

Updated

15 years ago
Attachment #118787 - Flags: review+

Comment 13

15 years ago
first patch i will remove

Updated

15 years ago
Attachment #118692 - Attachment is obsolete: true

Updated

15 years ago
Attachment #118787 - Flags: superreview?(kin)

Comment 14

15 years ago
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;
   }
Attachment #118787 - Flags: superreview?(kin) → superreview-

Comment 15

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

Comment 16

15 years ago
what is the latest on the patch for this one?

Comment 17

15 years ago
marking for next milestone
Target Milestone: --- → mozilla1.5alpha

Updated

15 years ago
Blocks: 193020

Comment 18

15 years ago
unsetting target milestone
Target Milestone: mozilla1.5alpha → ---
(Assignee)

Comment 19

15 years ago
-->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.
Assignee: mjudge → brade
Status: ASSIGNED → NEW
(Assignee)

Comment 20

15 years ago
Created attachment 127771 [details] [diff] [review]
clean up previous patch and make it apply to trunk
Attachment #118787 - Attachment is obsolete: true
(Assignee)

Comment 21

15 years ago
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)
Attachment #127771 - Flags: superreview?(dbaron)
Attachment #127771 - Flags: review+
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.
Attachment #127771 - Flags: superreview?(dbaron) → superreview+
(Assignee)

Comment 23

15 years ago
Created attachment 128174 [details] [diff] [review]
close to final patch (cut and pasted together so may not apply cleanly)
(Assignee)

Comment 24

15 years ago
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
Attachment #128174 - Attachment is obsolete: true
(Assignee)

Comment 25

15 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.