155871 - Textbox context menu generates spurious focus events.

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[[e-mail]]

As Dean Tessman noticed in his patch to bug 62495 and bug 96446, a context menu
click in a textbox generates extra focus events.

Unfortunately in fixing this I'm having to redo his URL Bar work :-(

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[[e-mail]]

I think this maintains the same click-selects-all behaviour as before, i.e.
1. Tabbing to URL Bar selects all
2. Mouse down in unfocused URL Bar starts a new selection
3. Mouse click in unfocused unselected URL Bar will select all

Comment 3

[:deanis74]

1.
-function URLBarMouseUpHandler(aEvent)
+function URLBarClickHandler(aEvent)
 {
-  if (gMouseDownX > -1 && Math.abs(aEvent.screenX - gMouseDownX) <= 2) {
-    // mouse has moved less than two horizontal pixels between mouse down
-    // and mouse up - call it a click
+  if (!gIgnoreClick && gClickSelectsAll && gURLBar.selectionStart ==
gURLBar.selectionEnd)
     gURLBar.select();
-    gMouseDownX = -1;
-  }
-}

I allowed two pixels of left/right play before selecting all.  You've changed
this to requiring no selection.  I can, though, select a character if you click
in the middle of it and move only one pixel.  Should these two be combined? 
Check that nothing's selected and the mouse has moved less than two pixels
horizontally?

2.
-function URLBarKeyupHandler(aEvent)
-{
-  if (aEvent.keyCode == aEvent.DOM_VK_TAB ) {
-    ShowAndSelectContentsOfURLBar();
-  }
 }

Is this not needed anymore?  Do we handle selecting the url bar when tabbing to
it somewhere else?

3. Does double-click still work when the url bar has focus to select the entire
contents?

4. You removed the blur handler, which did this:

-  // reset the selection so the next time the urlbar is focused it
-  // doesn't re-select the old selection
-  if (gClickSelectsAll && !gIgnoreFocus)
-    gURLBar.setSelectionRange(0, 0);

When I coded 62495 I found that I still needed this - it was there before I
touched anything.  If I remember correctly, it was to ensure everything was
selected when tabbing to the url bar.  Otherwise, if I had just a selection
highlighted and then tabbed off and tabbed back, the selection would remain
highlighted.

Comment 4

[:deanis74]

Neil, I just noticed that using the context menu key still selects everything. 
Would handling oncontextmenu instead of the right-click address this?

Comment 5

[[e-mail]]

Dean, thanks for your comments.
What I did was to remove all the urlbar handlers and add back as little as
possible after fixing the context focus event problem. This had the effect of
removing all of your code even if it wasn't directly related to the focus event
problem. I can't observe any problems relating to your third or fifth comments.

Comment 6

[:deanis74]

Comment on attachment 90282 [details] [diff] [review]
Proposed patch

I tested this and it all looks good.  The fixes for bug 62495 and the
right-click selection bug are maintained, and the remaining bug of the context
menu key selecting everything is also fixed.

You don't need the declaration of gMouseDownX anymore.	Remove that and r=me.

Note to super-reviewer: This doesn't change any existing functionality, it
fixes an underlying problem which I had worked around in my fix for bug 62495.

Comment 7

[[e-mail]]

Attachment: Removed gMouseDownX for review

Thanks Dean, good catch on that context menu key!

Comment 8

[jag (Peter Annema)]

Comment on attachment 90482 [details] [diff] [review]
Removed gMouseDownX for review

sr=jag

Comment 9

[[e-mail]]

Fix was checked in by [e-mail]

Comment 10

[Ian Nartowicz]

Did this first hit 1.3a?  I noticed a change in the behaviour when I installed
Build 2002120208.  Clicking in the URL box beyond the last character of the URL
no longer selects the URL, but instead places the caret at the end of the URL. 
Or was it caused by the Mac changes in bug 116441?  Either way the behaviour
changed and I am finding it irritating.

Comment 11

[John Morrison]

This changed due to the checkin in bug 116441, and I have reopened that bug,
since that is not the expected behaviour on win32.