131139 - Right-clicking or dragging a link clears selection

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect, P2)

Description

[Jesse Ruderman]

Steps to reproduce: 
1. Select some text or links.
2. Right-click any part of a link (selected or not selected).

Result: 
Selection is cleared, so I get a normal link context menu, which has Copy disabled.

What used to happen:
Get a context menu with Copy and Search as well as link options.

This regression happened between 030908 and 031008, so it's probably from
Aaron's fix for bug 66597.  The bug doesn't happen if the right end of the link
is selected but the left end isn't (!).

The same regression also breaks a pattern Mozilla copied from Netscape 4:
dragging a link, dropping it right away, and then dragging it again used to drag
the text of the link rather than the href.  Now the selection is cleared at the
beginning of the second drag.

In older builds, right-clicking never cleared a selection, even if the mouse was
outside the selection.  I think right-clicking outside of a selection should
clear the selection like in IE (maybe only when the right-click focuses something).

Comment 1

[Jesse Ruderman]

See also bug 56472, "focus not clearing highlighted text", currently marked as
invalid.  Bug 66597 was marked as depending on bug 56472.

Comment 2

[sairuh (rarely reading bugmail)]

terri, i'll take this one, if you want!

Comment 3

[Aaron Leventhal]

-> Pete Zha.

We discussed the fix on IRC.

Comment 4

[Pete Zha]

Attachment: Don't do MoveCaretToFocus if focus is caused by mouse

aaronl: I'm not sure it's the complete one(Just upload it as a workarround).
Maybe we need to use integer/enum as timeless said in bug 83496
(http://bugzilla.mozilla.org/show_bug.cgi?id=83496#c14). Because the focus may
gained by keyboard, mouse or script.
So, use integer/enum maybe usful for other code that need this logic.

Comment 5

[Aaron Leventhal]

Comment on attachment 75158 [details] [diff] [review]
Don't do MoveCaretToFocus if focus is caused by mouse

Good patch.

One possible change, if you decide:

I was thinking about comment 14 in bug 83496 
(http://bugzilla.mozilla.org/show_bug.cgi?id=83496#c14).

If you decide to use an enum that indicates fromMouse, fromApplication or
fromKeyboard for SetFocus(), then perhaps you should use that here as well,
now.

Comment 6

[Aaron Leventhal]

Comment on attachment 75158 [details] [diff] [review]
Don't do MoveCaretToFocus if focus is caused by mouse

Good patch.

One possible change, if you decide:

I was thinking about comment 14 in bug 83496 
(http://bugzilla.mozilla.org/show_bug.cgi?id=83496#c14).

If you decide to use an enum that indicates fromMouse, fromApplication or
fromKeyboard for SetFocus(), then perhaps you should use that here as well,
now.

Comment 7

[Pete Zha]

Attachment: Patch use integer as the parameter

Aaron: I add 3 status (NS_EVENT_FOCUSED_WITH_MOUSE, NS_EVENT_FOCUSED_WITH_KEY,
NS_EVENT_FOCUSED_WITH_APPLICATION) to nsIEventStateManager.h.

Comment 8

[Kathleen :Brade]

Maybe it's just me but...these flags seem a little confusing or simplistic.

I'd prefer to see these flags:
  NS_EVENT_FOCUSED_BY_CLICK
  NS_EVENT_FOCUSED_BY_CONTEXTMENU
  NS_EVENT_FOCUSED_BY_KEY
  NS_EVENT_FOCUSED_BY_APPLICATION

Adding a context menu focus should make it clearest (since context menus could
be invoked by either a mouse click or by keyboard).

Comment 9

[Pete Zha]

Attachment: Add more clearly flags.

brade: Thanks for suggestion. I have added those flags to
nsIEventStateManager.h
Could please give me an r=? Thanks

Comment 10

[Aaron Leventhal]

One thing I forgot:

When you focus by right clicking or dragging the link, and the old selection
remains, the next TAB press won't go to the right place.

That's because ::ShiftFocusInternal() uses the selection location to know where
to tab from.

I think we'll need a private member variable mLastFocusedWith, so that
::ShiftFocusInternal() can know whether it should use mCurrentFocus as the
starting location, instead of using the selection.

Pete, can you add that for me?

Comment 11

[Pete Zha]

Aaronl: Since this bug depends on 83496, can we first resolve that one before 
we deal with this one?

Comment 12

[Aaron Leventhal]

Pete, we can solve this one separately, by using the patch to ChangeFocus().

Use an enum instead of #define. Look around the code base to see how enums are
done first. Also, we don't need it to go 1, 2, 4, 8, ... like a bit flag, since
only 1 can be used at a time.

Comment 13

[Pete Zha]

Attachment: Patch(using enum to define flags)

Add mFocusedWith as private member.
Use enum to define flags.
Aaronl: please review it. Thanks

Comment 14

[Aaron Leventhal]

This looks good to me. Bryner?

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 76379 [details] [diff] [review]
Patch(using enum to define flags)

>+enum FocusedWithType {
>+  NS_EVENT_FOCUSED_BY_UNKNOWN     = 0x0, // focus gained via unknown method
>+  NS_EVENT_FOCUSED_BY_CLICK       = 0x1, // focus gained via mouse click
>+  NS_EVENT_FOCUSED_BY_KEY         = 0x2, // focus gained via key press (like tab)
>+  NS_EVENT_FOCUSED_BY_CONTEXTMENU = 0x3, // focus gained via context menu
>+  NS_EVENT_FOCUSED_BY_APPLICATION = 0x4  // focus gained via Application (like script)
>+};
>+

No need to explicitly define the enum values if they are sequential starting
from 0.

>-  if (itemType != nsIDocShellTreeItem::typeChrome) {   // If not content, forget selection - just use mCurrentFocus
>+  if (itemType != nsIDocShellTreeItem::typeChrome && mLastFocusedWith != NS_EVENT_FOCUSED_BY_CLICK) {
>+  // If not content or focused gained via mouse click, forget selection - just use mCurrentFocus
>     // we're going to tab from the selection position 

Fix indenting of the comment.

With those changes, r=bryner.

Comment 16

[sairuh (rarely reading bugmail)]

also see this on current trunk builds for linux rh7.2 and mac 10.1.3 --although
the dragging issue seems different among the platforms:

* win2k and linux: if i drag on the non-link portion of the selection, the
selection stays. but if i drag on the link portion of the selection, the
selection goes away for the everything but the *entire* linked text. in other
words, if only part of a link was selected, the non-link selection goes away and
the *entire* linked text becomes selected.

* on mac, dragging on the non-link portion of the selection makes the entire
selection go away. but dragging on the link portion behaves the same as on win2k
and linux.

using a test build from aaronl today on win2k (which contains a fix for this),
it looks like this bug is resolved. i tested jesse's recipe wrt right-clicking,
including both right-end and left-end link selections, and the selection is
maintained.

and for dragging: the selection is maintained if i drag on either the linked or
non-linked portion of the selection.

Comment 17

[Pete Zha]

Attachment: Patch (with bryner's comments)

Fix intend problem and remove value set in enum define

Comment 18

[Pete Zha]

Attachment: The right one

Sorry, I uploaded the wrong patch. Please ignore the last one

Comment 19

[Pete Zha]

Attachment: The right one's sister, more beautiful :-)

Chat with aaron in IRC. I should use following code to define enum.
+enum EFocusedWithType {
+  eEventFocusedByUnknown,     // focus gained via unknown method
+  eEventFocusedByMouse,       // focus gained via mouse
+  eEventFocusedByKey,	       // focus gained via key press (like tab)
+  eEventFocusedByContextMenu, // focus gained via context menu
+  eEventFocusedByApplication  // focus gained via Application (like script)
+};
+
need r= Thanks

Comment 20

[Aaron Leventhal]

Comment on attachment 76511 [details] [diff] [review]
The right one's sister, more beautiful :-)

r=aaronl

Comment 21

[Aaron Leventhal]

Kin, I checked clicking in content between links. THat still works - the next
tab moves relative to there.

Comment 22

[kinmoz]

Comment on attachment 76511 [details] [diff] [review]
The right one's sister, more beautiful :-)

This patch is slightly different then the one that saari commented on in bug
83496, but I'm assuming it's still ok with him that this goes in as a temporary
solution.

sr=[e-mail]

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 76511 [details] [diff] [review]
The right one's sister, more beautiful :-)

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 24

[Aaron Leventhal]

checked in

Comment 25

[sairuh (rarely reading bugmail)]

vrfy'd fixed using 2002.04.09 comm bits on linux rh7.2, win2k and mac 10.1.3.

the only exception is that on Mac, the selection is still cleared away when i
drag on non-linked content --filed bug 136457 for that.