Last Comment Bug 28583 - Focusing text widget (except on click) should select widget contents [also form <input> field]
: Focusing text widget (except on click) should select widget contents [also fo...
Status: VERIFIED FIXED
[behavior]
: access, polish
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: P3 normal with 4 votes (vote)
: mozilla1.0
Assigned To: Aaron Leventhal
: sujay
Mentors:
: 23276 25458 30230 36278 37769 60610 62880 66284 69310 73078 74003 79344 81377 89339 96652 131185 141869 155683 163232 170576 (view as bug list)
Depends on: 83496 90587 127272
Blocks: 49574 89689 104166 atfmeta 172203
  Show dependency treegraph
 
Reported: 2000-02-19 22:31 PST by Matthew Paul Thomas
Modified: 2009-06-22 12:54 PDT (History)
42 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rough patch (1.18 KB, patch)
2000-05-02 15:13 PDT, Eric Pollmann
no flags Details | Diff | Splinter Review
SelectAll When ShiftFocus (1.02 KB, patch)
2002-03-05 02:34 PST, Jim Song
no flags Details | Diff | Splinter Review
New Patch (1.23 KB, patch)
2002-03-08 01:05 PST, Jim Song
no flags Details | Diff | Splinter Review
Patches with Checking Text Widgets (1.56 KB, patch)
2002-03-11 19:19 PST, Jim Song
no flags Details | Diff | Splinter Review
Patch just add SelectAll in nsHTMLInputElement.cpp (619 bytes, patch)
2002-03-19 03:10 PST, Pete Zha
no flags Details | Diff | Splinter Review
Basic XBL-based patch (1.48 KB, patch)
2002-08-22 05:23 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Proposed patch (1.96 KB, patch)
2002-08-23 01:54 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Uses esm:MoveCaretToFocus to check for textfield/password controls and select them (11.17 KB, patch)
2002-10-02 14:22 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Same thing, but with one more null check on mCurrentFocus (11.15 KB, patch)
2002-10-02 19:10 PDT, Aaron Leventhal
brade: review+
Details | Diff | Splinter Review
New patch with brade's changes (11.16 KB, patch)
2002-10-03 12:18 PDT, Aaron Leventhal
aaronlev: review+
Details | Diff | Splinter Review
Oops, forgot to change PRInt32 -> PRInt8 for static in both places (11.15 KB, patch)
2002-10-03 12:23 PDT, Aaron Leventhal
aaronlev: review+
Details | Diff | Splinter Review
Comments added to nsLookAndFeel.cpp, explaining what the mysterious values are (12.28 KB, patch)
2002-10-03 12:53 PDT, Aaron Leventhal
aaronlev: review+
Details | Diff | Splinter Review
Better comments (12.35 KB, patch)
2002-10-03 13:12 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Better comments (12.33 KB, patch)
2002-10-03 13:17 PDT, Aaron Leventhal
aaronlev: review+
sfraser_bugs: superreview+
Details | Diff | Splinter Review

Description Matthew Paul Thomas 2000-02-19 22:31:47 PST
Build: 2000021608, MacOS 8.6

To reproduce:
* Open the Bookmarks window.
* Open the Properties window for a bookmark.
* If there is nothing in the Location field for the bookmark (it doesn't seem to
  be hooked up yet), type some text into it.
* Place the cursor in the Name field.
* Press Tab.

Or:
* Open the prefs window, and navigate to Advanced > Proxies.
* Select the `Manual proxy configuration' radio button.
* Place the cursor in the FTP proxy server name field.
* Press Tab.

What should happen (on all platforms):
* The next widget, which is a text field, is given focus and its entire contents
  is selected (so that any new typing will overwrite the existing contents).

What actually happens:
* The text field is given focus, but its contents are not selected.
Comment 1 Peter Trudelle 2000-02-19 23:29:43 PST
reproduced in recent verification build on Win98. reassigning to pollmann. Eric, 
is this part of the tab mgr work I hear you are doing?
Comment 2 Peter Trudelle 2000-02-25 13:57:28 PST
*** Bug 23276 has been marked as a duplicate of this bug. ***
Comment 3 lchiang 2000-02-25 14:29:46 PST
to qa: when this bug is fixed, pls notify esther or lchiang so we can try our 
dup bug case. Thanks.
Comment 4 Peter Trudelle 2000-02-28 13:07:52 PST
*** Bug 25458 has been marked as a duplicate of this bug. ***
Comment 5 John Morrison 2000-03-03 11:21:22 PST
*** Bug 30230 has been marked as a duplicate of this bug. ***
Comment 6 Eric Pollmann 2000-04-10 21:19:43 PDT
Minor usability issue, scheduling for M20!
Comment 7 Brendan Donohoe 2000-05-01 13:52:52 PDT
Not so minor, IMO.  And it's all the little "minor" details that make a product 
feel good.  Skip the minor details and you wind up with a rather frustrating 
product.  Why do you think MS IE 4.5 and 5 did so well on the Mac?  It 
wasn't because they had Microsoft's name behind them...

Just registering my UI vote so these things don't slip through the cracks.
Comment 8 Stanton McCandlish 2000-05-01 15:23:08 PDT
This bug is probably a duplicate of or at least related to bug 36278 (which has 
been incorrectly classified as a duplicate of 31809 and 33069).  See also 29086
Comment 9 Asa Dotzler [:asa] 2000-05-02 05:52:00 PDT
*** Bug 37769 has been marked as a duplicate of this bug. ***
Comment 10 Eric Pollmann 2000-05-02 15:03:44 PDT
Reassigning to you Steve - this looks like it's more your area.

I tried a first pass at implementing this a while back and ran into lots of 
focus issue and other problems. (will attach patch, which was only a few line 
change to nsGfxTextControlFrame.cpp)

1) The text was selected only the first time the widget received focus
2) The text was not deselected when the widget lost focus.
3) Clicking on an empty widget caused asserts to fire
4) Peformance of clicking on a widget dropped significantly.
Comment 11 Eric Pollmann 2000-05-02 15:13:08 PDT
Created attachment 8221 [details] [diff] [review]
rough patch
Comment 12 Eric Pollmann 2000-05-02 15:16:29 PDT
*** Bug 36278 has been marked as a duplicate of this bug. ***
Comment 13 Eric Pollmann 2000-05-02 15:18:47 PDT
The reason for problem 2) is that nsGfxTextControlFrame::SetFocus() never gets 
called with aOn false - meaning, we're never notified when the text control 
looses focus.  This could be fixed by just moving the Deselect() call to 
wherever we get that notification.
Comment 14 karnaze (gone) 2000-05-17 15:13:27 PDT
Reassigning to beppe (editor).
Comment 15 rubydoo123 2000-05-18 09:39:31 PDT
assigning to Kin for review
Comment 16 kinmoz 2000-05-18 15:41:42 PDT
Accepting bug.
Comment 17 Matthew Paul Thomas 2000-05-19 20:08:59 PDT
Hey, I protest. This isn't an RFE, it's a Bug with a capital B.

W.r.t. Eric's comments -- the widget contents should only be selected when the 
widget is tabbed to, *not* when it is clicked in. Clicking should just position 
the caret, as it would if the widget already had focus.
Comment 18 rods (gone) 2000-05-22 08:17:30 PDT
Tabbing into a textfield in WinNt Nav 4.x doesn't select the content, but the 
textfield does remember where the cursor was (that could be a bug)
Comment 19 Decklin Foster 2000-06-01 21:39:30 PDT
I'd like to add that this behavior should *not* be enabled on Linux and other
X11-using platforms, as it interferes with the X selection/paste mechanism.

Comment 20 Matthew Paul Thomas 2000-06-04 06:47:12 PDT
Decklin, does GTK really not do this when you tab to a text widget? If so, that 
means pp. I think it's possible that resetting the X selection when tabbing to a 
text widget is actually desirable behavior anyway.

Raising severity to Minor after rereading
<http://bugzilla.mozilla.org/bug_status.html#severity>. Nominating for RTM, as I 
can't believe that a commercial product would go out the door with this unfixed.
Comment 21 Decklin Foster 2000-06-04 23:01:39 PDT
*doublechecks*
Yes, GTK really does not do it. I probably would have complained already if it
did ;-)
Comment 22 rubydoo123 2000-07-27 14:17:08 PDT
adding help wanted keyword
Comment 23 Matthew Paul Thomas 2000-08-27 19:12:41 PDT
Not an RFE --> removing `[RFE]' from summary. Given the number of times it 
happens in normal usage, not a minor issue either --> normal.
Comment 24 Jesse Ruderman 2000-11-19 01:03:32 PST
*** Bug 60610 has been marked as a duplicate of this bug. ***
Comment 25 Matthew Paul Thomas 2000-12-26 23:40:36 PST
Ok, refining summary based on comments in bug 63705.

The contents of a text field should be selected if the field receives focus by
any method *except* clicking in it (clicking in it should just place the caret).
This includes tabbing to the field, and setting focus to the field when the
dialog is created.
Comment 26 Matthew Paul Thomas 2001-01-23 17:29:49 PST
*** Bug 66284 has been marked as a duplicate of this bug. ***
Comment 27 Zach Lipton [:zach] 2001-03-29 17:10:01 PST
*** Bug 74003 has been marked as a duplicate of this bug. ***
Comment 28 Kathleen Brade 2001-04-16 12:04:28 PDT
*** Bug 69310 has been marked as a duplicate of this bug. ***
Comment 29 Kathleen Brade 2001-04-16 12:11:22 PDT
remove milestone for reconsideration, add some keywords

note:  on Linux, the correct behavior seems to be to place the caret at the end 
of the edit field.  Macintosh and Windows expects the fields to be selected.  

As for 4.x behavior on Windows, 4.x is inconsisent (if I recall correctly).  In 
Composer dialogs, the tabbing among the various edit fields does a select all.
Comment 30 kinmoz 2001-04-16 17:17:11 PDT
Set to mozilla0.9.1 for now. Cc'ing cmanske who had the dup of this bug.
Comment 31 Simon Fraser 2001-04-19 11:12:00 PDT
We probably need an nsILookAndFeel entry for this. I so wish it would work on 
Mac.
Comment 32 Kathleen Brade 2001-05-08 08:44:19 PDT
*** Bug 79344 has been marked as a duplicate of this bug. ***
Comment 33 Jacek Piskozub 2001-05-31 12:12:59 PDT
11 dups. marking mostfreq.
Comment 34 Jesse Ruderman 2001-05-31 12:29:02 PDT
*** Bug 73078 has been marked as a duplicate of this bug. ***
Comment 35 Jesse Ruderman 2001-05-31 12:40:24 PDT
*** Bug 81377 has been marked as a duplicate of this bug. ***
Comment 36 Matthew Paul Thomas 2001-06-05 08:41:00 PDT
*** Bug 83588 has been marked as a duplicate of this bug. ***
Comment 37 rubydoo123 2001-06-12 16:39:12 PDT
well, we would like to get this in for 9.2, but until bug 83496 gets resolved, 
this one can't. Moving over to 1.0 but will gladly move in once that otehr bug 
gets fixed
Comment 38 Jesse Ruderman 2001-07-05 15:13:10 PDT
*** Bug 89339 has been marked as a duplicate of this bug. ***
Comment 39 Matthew Paul Thomas 2001-07-08 12:15:42 PDT
*** Bug 62880 has been marked as a duplicate of this bug. ***
Comment 40 Kathleen Brade 2001-08-23 11:27:54 PDT
*** Bug 96652 has been marked as a duplicate of this bug. ***
Comment 41 Boris Zbarsky [:bz] 2001-09-30 08:44:52 PDT
*** Bug 62880 has been marked as a duplicate of this bug. ***
Comment 42 Matthew Paul Thomas 2001-10-15 17:07:06 PDT
Removing pp keyword. I discussed this bug with someone whose specialty is the X 
selection mechanism, and his response was basically `RTFICCCM'. (You can do so 
at <http://tronche.com/gui/x/icccm/>.) There's nothing wrong with selecting the 
contents of a text field on X when tabbing to it or programmatically focusing 
it, as long as it is made the SECONDARY selection, not the PRIMARY one. The bug 
where tabbing to a GTK+ text field doesn't select its contents is fixed in GTK+ 
2.0 <http://bugzilla.gnome.org/show_bug.cgi?id=57743>.
Comment 43 kinmoz 2001-11-27 15:26:00 PST
Bulk move of mozilla1.0 bugs to mozilla.1.0.1. I will try to pull some of these
back in if I can.
Comment 44 Peter Trudelle 2001-12-10 23:22:52 PST
nominating for nsbeta1, adding access keyword
Comment 45 Kevin McCluskey (gone) 2002-02-12 11:38:36 PST
Marking nsbeta1+
Comment 46 Kevin McCluskey (gone) 2002-02-12 13:26:52 PST
Rod, can you take a look at this?
Comment 47 Kevin McCluskey (gone) 2002-02-19 09:42:06 PST
nsbeta1-. Engineers are overloaded with higher priority bugs. 
Comment 48 Peter Trudelle 2002-02-19 09:55:05 PST
cc marlon for UE input.  Is there anyone else who could take this?  It seems
like fundamental usability correctness, and may be important for accessibility
too, especially IE compatibility.
Comment 49 Kevin McCluskey (gone) 2002-02-22 10:16:47 PST
Kin and Rod think the editor changes should be easy to implement once bug 83496
is fixed.

Marking this bug nsbeta1+ and nominating bug 83496 for nsbeta1
Comment 50 Aaron Leventhal 2002-02-22 10:49:25 PST
I would know how to do this without a fix for bug 83496.

It would require a modification to nsEventStateManager::ShiftFocus().
It would simply check to see if the user had moved onto something with it's own
caret, if so it would select the contents.

If people don't think that's too much of a hack I can take this one.
Comment 51 Simon Fraser 2002-02-22 13:04:40 PST
"had moved onto something with it's own caret"

Can you explain?
Comment 52 Aaron Leventhal 2002-02-22 14:45:27 PST
textfields and textareas are editable, and as such have their own
caret/selection that is separate from the document's caret/selection.
Comment 53 Kathleen Brade 2002-02-22 14:56:44 PST
Be sure you test any fix with the urlbar (which does some funky things (imo) to
selection.

Can we also use ShiftFocusInternal in the case where the window just opens?

Lastly, this should be a hidden preference or similar.  Unix users expect a
caret (current behavior), not a selection while Macintosh and Windows users
expect there to be a selection.
Comment 54 Simon Fraser 2002-02-22 15:11:14 PST
Aaron: how can you use the 'thing has caret' information to decide whether the 
user just focussed it with a tab key or a click (which is what this bug is 
about)?
Comment 55 Jim Song 2002-03-05 02:34:49 PST
Created attachment 72569 [details] [diff] [review]
SelectAll When ShiftFocus

Aaron,
I propose the patch according to your suggestion before.
Function ShiftFocus will only be called when Tab is used.
So the patches can solve the problem in Bookmark property 
dialog and the preference dialog etc. 

Of course I have tested the patch, and it works.
Comment 56 Kathleen Brade 2002-03-05 06:35:10 PST
Comment on attachment 72569 [details] [diff] [review]
SelectAll When ShiftFocus

One problem with this patch is that it appears to be always doing the
select-all behavior.  This needs to be platform-specific (Linux expects to have
a caret at the end of the text rather than the selectAll behavior expected on
Macintosh).

jim.song@sun.com--do you need help revising this patch to account for that?  I
envision the patch will make use of a hidden preference which Linux will set
differently from other platforms.  Let me know.
Comment 57 Kathleen Brade 2002-03-05 07:14:58 PST
A few other notes/comments about the patch:
  * please use consistent code style for the code you are editing
    (put spaces in the same places as surrounding code)
  * add a comment about what the block of code (which you are adding) is doing
  * it seems to work great (hurray!) except I don't see a caret when I tab to an
empty textarea.
  * Also, occasionally when I click I see a flash where the text seems to select
and then the caret is placed and the selection is removed.  

steps to reproduce "flicker":
  * load this bug in a browser window
  * click in the QA context text field
  * tab until you get to the textarea for Additional Comments
     (notice there isn't a caret)
  * click in a textfield which has text in it (such as blocker list)
     (watch carefully for flicker)
  * click in other textfields that have text in them; see flicker
Subsequent clicks have no flicker unless the field has a selection in it prior
to blur.  The flicker is NOT a regression introduced by this patch.

I'm not sure about the lack of a caret in a textarea; I think that should be
addressed (as well as the preference for Linux).
Comment 58 Aaron Leventhal 2002-03-05 09:25:48 PST
Couple of notes:

Jim, Looks like great progress!

Please emember to remove the comment with your name in the final patch. Patches
that checked in shouldn't have personal notes left in them.

Be aware there may be a conflict with a patch I am checking in for bug 66597.
That patch will be checked in this week (hopefully tonight).
Comment 59 Simon Fraser 2002-03-05 11:09:09 PST
I'm somewhat uneasy about this patch: my feeling is that the ESM is not the
right place to be putting SelectAll() behaviour, preffed or not.
Comment 60 Jim Song 2002-03-05 18:48:43 PST
Hi, Thanks!
No Caret if blank:
   My fault. Will give patch tomorrow.
Flicker:
   Not introduced by this patches. So need another bug?
No Selection on Linux:
   As Mattew said before, Gtk2.0 require Selection when tabbing.
   http://bugzilla.gnome.org/show_bug.cgi?id=57743

Comment 61 Jim Song 2002-03-08 01:05:01 PST
Created attachment 73178 [details] [diff] [review]
New Patch

I have added code to show the cursor if the content is empty.
Please review.
Comment 62 kinmoz 2002-03-08 07:46:15 PST
Comment on attachment 73178 [details] [diff] [review]
New Patch

Like sfraser, I'm a bit uneasy about placing this in ESM.

When I look at this patch, I'm wondering to myself, what happens if
mCurrentTarget isn't a text widget, what selection controller is returned? The
one for outer document? If so, will the select all cause the entire outer doc
to get selected?

Also does getting the "value" attribute on a TextArea really work? Getting the
value of a text widget as a string is not cheap. If we could just distinguish
how we got focus, nsGfxTextControlFrame or the editor could just peak at the
number of children underneath the root node.
Comment 63 rubydoo123 2002-03-08 12:08:01 PST
removing myself from the cc list
Comment 64 Jim Song 2002-03-10 22:35:45 PST
Hi, Kin,

>If mCurrentTarget isn't a text widget, what selection controller is 
>returned? The one for outer document? 
Do you think it's right to return the document's selection controller
for a widget?

I think we can check whether it is a text widget if it is needed, but
if there is the case to tab to a document as a whole?


Comment 65 Jim Song 2002-03-11 00:13:51 PST
Sorry for my upper unclear comments.
For Kin's uneasy, I wonder if there is the case that, a control is tabbed into, 
and its selection control is from the outer document(as Kin said)?

If tabbing and select only apply to text widget, I also wonder if we can check the 
frame type, if it is text widget, then select it?

Comment 66 Aaron Leventhal 2002-03-11 00:45:24 PST
// The following code is a generic way to check if we're in a text field 

nsCOMPtr<nsIFrameSelection> frameSelection, docFrameSelection;
GetSelection(mCurrentTarget, mPresContext, getter_AddRefs(frameSelection));
mPresShell->GetFrameSelection(getter_AddRefs(docFrameSelection));

if (docFrameSelection && (frameSelection != docFrameSelection)) {     
  // If we get here we are in a textfield or textarea,
  // because they have their own frame selection, separate from the document's
}
Comment 67 Jim Song 2002-03-11 19:19:11 PST
Created attachment 73621 [details] [diff] [review]
Patches with Checking Text Widgets

Add Aaron's codes to check if it's a text field.
A small change from Aaron's is:
if( frameSelection && ... )
Comment 68 Damian Yerrick 2002-03-11 20:46:03 PST
Mozilla should handle single-line inputs and textareas differently.
Here are some situations where selecting the entire contents of a
textarea on focus would be a bad idea:

Situation 1
1. Log in to http://slashdot.org/ or http://www.kuro5hin.org/
2. Open a story
3. Click "Reply to This" under a comment
4. Compose a reply
5. Click Preview
6. Tab to field

Situation 2
1. Log in to http://www.wikipedia.com/
2. Click "Recent Changes" to get a list of articles
3. Find an article that looks interesting and click its title
4. Click "Edit this page"
5. Tab to the edit box containing the article's data

Situation 3
1. Fill in the Bugzilla Helper form
2. Submit the form to create a new bug report
3. Tab to the textarea

Selecting the entire contents of a textarea on focus would be roughly
equivalent to selecting the entire contents of a newly opened Composer
page.  I see no reason why the user would want to replace the entire
contents of a textarea with one keystroke.  If the user really wants
to do that, it's as easy as Accel+A.
Comment 69 Jim Song 2002-03-11 21:12:35 PST
Yes, Danmian, I think you are quite right.
Comment 70 Jim Song 2002-03-11 21:13:28 PST
Yes, Damian, I think you are quite right.
Comment 71 Peter Trudelle 2002-03-11 23:39:27 PST
I agree. Opera selects the contents of textareas you tab into, which seems
wrong, even though it is what you expect in a textfield.  IE6 and NS4.x both put
a caret at the end of the text, which IMO is what mozilla should do too.
Comment 72 Peter Lairo 2002-03-12 01:29:35 PST
Best Solution:

1. Single-line inputs   - entire text is selected
2. Multi-line textareas - curser placed at end of existing text

1. Allows to quickly select/replace small (default) entries. Accidental deletes
are quickly repaired (few text).
2. Allows to edit text without the risk of accidentally deleting (and then
having to rewrite) large amounts of text.
Comment 73 Jim Song 2002-03-12 02:51:56 PST
Hi,
Does selection only apply to widget with localname as:
text and input? If so, we can only select content with
such names.
Comment 74 timeless 2002-03-12 04:55:32 PST
nc4 remembers where in an input widget your cursor was. Eg if I click the a in 
the status whiteboard [beh_avior] then tab out and shift tab back in, my cursor 
is placed at beh_avior, which is what I expect. The same applies for textareas.
Comment 75 Aaron Leventhal 2002-03-12 08:42:58 PST
The CSS "display:" attribute can be used to make any element display as a text
input, so checking the frame type is the best way.
Comment 76 Simon Fraser 2002-03-12 13:48:15 PST
I agree with timeless. We should be able to remember where the selection was in
a textarea.
Comment 77 Samuel Sieb 2002-03-13 14:43:19 PST
Mozilla does currently remember position and selection state when tabbing
through textboxes.
Comment 78 Boris Zbarsky [:bz] 2002-03-15 19:42:08 PST
*** Bug 131185 has been marked as a duplicate of this bug. ***
Comment 79 Pete Zha 2002-03-18 23:30:22 PST
I think we should deal it more easier.
Select all if it is a singel-line text field, and do nothing but leave it's old 
selection and cursor position if it is a textarea.
Just like what IE does.
Comment 80 Pete Zha 2002-03-19 03:10:43 PST
Created attachment 74933 [details] [diff] [review]
Patch just add SelectAll in nsHTMLInputElement.cpp

Just add SelectAll(aPresContext); to nsHTMLInputElement.cpp
Because it is the text field and xul input element's implementation.
I tested it on windows with pref and html form element. 
This select action is only for text and password, and it will not effect the
textarea element
Comment 81 Axel Hecht 2002-03-19 04:35:55 PST
talking to petez on #mozilla the current patch behaves like selecting with the
mouse on X. That's bad.
The mouse selection (X clipboard?) is holy, don't mess with it without an 
explicit user request.
On windows, an explicit action is needed to move the selection to the clipboard.
On X, the selection is automatically copied to the clipboard. That must not
happen as a side effect of moving focus.
Comment 82 Kathleen Brade 2002-03-19 06:44:22 PST
Comment on attachment 74933 [details] [diff] [review]
Patch just add SelectAll in nsHTMLInputElement.cpp

This patch needs a lot more work.  At a minimum *any* fix for this bug MUST
have a preference (hidden).  It must default to on for Macintosh and Windows
users and off for Linux users.	I expect to see a preference added to all.js as
well as the linux-specific file.

The other big problem with this patch (maybe I just need more context?) is that
it is ALWAYS does a SelectAll.	That is NOT the correct behavior if one clicks
in the field (tabbing should do SelectAll; clicking should position caret).
Comment 83 Charles Manske 2002-03-19 08:37:14 PST
I agree with brade. We must NOT do this on all setfocus events, i.e., when
user clicks in an input field. I would have fixed it years ago if it were that 
simple! The tricky part when I first investigated this problem was how to 
figure out when focus was set just by tabbing into a text input.
That is the only time we should select. And even that behavior is platform
specific, as brade points out. A pref would also be good so users could change 
the behavior -- many don't like it to select at all, even if it is the 
default platform behavior.
Comment 84 Aaron Leventhal 2002-03-19 19:55:37 PST
I believe I have the right fix for this bug, by implementing bug 83496.

Taking.
Comment 85 Aaron Leventhal 2002-03-19 20:47:49 PST
-> Pete Zha 

We discussed the correct fix on IRC.
Comment 86 Matthew Paul Thomas 2002-03-20 00:46:32 PST
As described in comment 42 and comment 60, this bug is *not* platform-specific. 
That it didn't work properly in GTK 1.x was a bug which has been fixed. Like 
any other selection, this selection should be copied to PRIMARY, but *not* to 
CLIPBOARD.
Comment 87 Axel Hecht 2002-03-20 01:56:30 PST
I would like to note that the behaviour of selection and clipboard is platform
dependent. At least on solaris, you have to learn by heart which app puts it's
stuff where. As far as clipboard goes, X is not really *one* platform.
Whatever patch comes, this should get rich QA on as many X-servers as possible,
and check out different toolkits. (Adding Roland for the xlib port)
Comment 88 Roland Mainz 2002-03-20 02:09:48 PST
There is actually one behaviour preferred on X11 - that one of Motif2 (not the
old Motif1 one (e.g. NS4.x is Motif1.x-based)).
Ignore the rest - just implement it like Motif2/CDE2.x does the job.
Comment 89 Matthew Paul Thomas 2002-03-20 05:44:45 PST
> on solaris, you have to learn by heart which app puts it's stuff where.

That's not platform-dependent, that's some apps being broken. For example, 
xchat still incorrectly sets CLIPBOARD on selection, while OpenOffice 
incorrectly does nothing on selection and sets PRIMARY on Copy. Most other 
common apps in active development have been fixed.
Comment 90 Axel Hecht 2002-03-20 06:03:25 PST
note: the current gtk requirements are 1.2.*, don't change that without
separate approval. I suggest making the fix for win and mac only if we can't
fix this without changing the requirements.
Comment 91 Matthew Paul Thomas 2002-03-20 06:25:43 PST
No, this has nothing to do with our gtk requirements. It's a bug in *Mozilla's* 
text controls, which should be fixed no matter what version of gtk is 
underneath. As an analogy, the Windows 95/98 native text controls have line-
-wrapping bugs which are not present in Windows 2000; that does *not* mean that 
Mozilla's own text controls should emulate those bugs when on Windows 95/98.
Comment 92 Akkana Peck 2002-03-20 11:26:59 PST
So, Matthew: you're saying that because gtk is planning to change their behavior
in an upcoming release, which very few people have actually tried running (do
any distros ship with it yet?  Redhat 7.2 uses gtk 1.2), and which mozilla
itself is not using, we should completely change the selection behavior of text
fields, making it incompatible with the ICCCM (see comment 42, where you
yourself mention that the ICCCM says tabbing into text fields should not make
the contents the primary selection).  If we changed the code accordingly,
selecting the text field contents but NOT autocopying it to the clipboard, we
become compatible with the ICCCM but become very confusing for Unix users to
use, since now to get the contents into the primary selection to make it
available to paste, users will have to select it again, which isn't visually
obvious since it looks like it's already selected, and which can't be done by
the usual drag-select method since dragging inside a mozilla selection invokes
drag-and-drop code rather than selection code.

(BTW, does gtk 2.0 copy this selected text to the X primary selection?  You
haven't mentioned that.)

Imposing this sort of confusion, which is seen in no other currently shipping
Unix app or toolkit, upon all Unix users for mozilla 1.0, with no prior warning
so that users have a chance to complain (which they will, I assure you) is wrong.
Comment 93 Matthew Paul Thomas 2002-03-20 17:32:25 PST
Ok, I made a mistake in comment 42. Sorry.
`as long as it is made the SECONDARY selection, not the PRIMARY one'
should be
`as long as it is made the PRIMARY selection, not the CLIPBOARD one'

To address your other points:
1.  The GTK+ developers have already fixed this bug in GTK+ 2.0. There are *no*
    plans to `change their behavior in an upcoming release'.
2.  As I already said, which version of GTK Mozilla itself requires is
    irrelevant to this bug, since Mozilla rolls its own UI controls, and
    shouldn't replicate bugs in old versions of other toolkits.
3.  Fixing this bug does not `completely change the selection behavior of text
    fields', just what happens when you tab into them.
4.  `Selecting the text field contents but NOT autocopying it to the clipboard'
    should not be `very confusing for Unix users to use', because no app should
    *ever* autocopy selected text to the clipboard. As I said, xchat is the
    only major app still broken in that particular fashion.
5.  Given that Mozilla 1.0 and GNOME 2.0 are likely to be released within a
    couple of months of each other, and therefore distributed together in Linux
    distributions, the number of people using Mozilla 1.x with GNOME 2.x will
    probably be several times greater than the number of people using it with
    GNOME 1.x.

> does gtk 2.0 copy this selected text to the X primary selection?

Yes it does.
Comment 94 Kee Hinckley 2002-03-20 17:50:56 PST
Perhaps I should submit another bug report on this, but it relates rather
closely to this discussion.  On MacOS X when you drag select in the URL area it
does a select-all instead of selecting the region dragged.  In other words, if I
have:
   http://bugzilla.mozilla.org/show_bug.cgi?id=28583
and I want to change the id to something else, so I try and drag select it, I
end up selecting the whole thing. The behavior in forms is correct, it's only in
the URL area that it's wrong.  Double click behavior on the other hand, which
should select a word, behaves very oddly with URLs (whether in the URL entry
area, or a text region like this one.  It seems to select from the click point
back to the first space or beginning of line, and it doesn't treat punctuation
as a word delimiter.  (There are also some pending delete bugs in textareas, but
I'll check and see if they've been reported already.)

If I should submit a separate report feel free to yell at me and tell me so.
Comment 95 Simon Fraser 2002-03-20 17:57:09 PST
Kee: that's bug 116441.
Comment 96 Akkana Peck 2002-03-21 17:59:06 PST
gtk 2.0 is an upcoming release to most users, since no distro is actually
shipping apps which behave this way yet.  We have no idea what user reaction to
this move will be.  Most users will download mozilla 1.0 long before they
reinstall linux and see gtk 2.0, so mozilla will be where they first see this
behavior, and mozilla will appear to be broken in comparison with everything
else on their system.

> 3.  Fixing this bug does not `completely change the selection behavior of text
>     fields', just what happens when you tab into them.

It "completely changes the selection behavior of text fields when you tab into
them."  Clearer?

> because no app should *ever* autocopy selected text to the clipboard.

I was referring to the mozilla clipboard, nsClipboard, which is the class used
for copying selections to PRIMARY as well as CLIPBOARD.  Your earlier ICCCM
quote had said we shouldn't autocopy to PRIMARY, and I was responding to that.
Comment 97 Val Henson 2002-03-21 20:45:56 PST
With regard to the argument that GTK+ 1.2's behavior (no select-on-tab) is a bug:

http://www.gtk.org/gtk-2.0.0-notes.html
The new default 2.0 behavior of select-on-tab is described as a way to "improve
usability", not a bugfix.  Note also that the instructions to reconfigure GTK+
back to the 1.2 behavior are included on the first page of the GTK+ 2.0 release
notes.

Summary: The "old" behavior is NOT a bug or broken.  Making select-on-tab the
default behavior is fine as long as there is an obvious way to reconfigure it to
have the "old" behavior.
Comment 98 Boris Zbarsky [:bz] 2002-05-02 16:30:56 PDT
*** Bug 141869 has been marked as a duplicate of this bug. ***
Comment 99 Boris Zbarsky [:bz] 2002-07-03 18:06:56 PDT
*** Bug 155683 has been marked as a duplicate of this bug. ***
Comment 100 Luke Morey 2002-08-07 08:09:06 PDT
Yay!  So glad I found this bug...drives me bonkers every day.

There's something related.  When focusing a text field, the cursor's flashing
should start with the cursor /visible/.  At the moment, when tabbing into a text
field, not only is the content not highlighted, the cursor isn't even showing. 
So I don't know if I've hit tab enough times to get where I'm going.  It's only
a second to wait, but if I'm tabbingn through to the next widget, there should
be visual feedback for every widget I tab to.

Does that need a separate bug...?
Comment 101 Aaron Leventhal 2002-08-07 10:11:17 PDT
Mozilla@strangemonkey - I think in most programs the cursor starts as visible
right after any key is pressed,not just tab. That would be a separate bug, but
check to see if one already exists before filing a new one.
Comment 102 Jesse Ruderman 2002-08-19 15:06:26 PDT
*** Bug 163232 has been marked as a duplicate of this bug. ***
Comment 103 neil@parkwaycc.co.uk 2002-08-22 05:23:47 PDT
Created attachment 96297 [details] [diff] [review]
Basic XBL-based patch

Note that this only affects XUL textboxes, not editable menulists or HTML.
Comment 104 neil@parkwaycc.co.uk 2002-08-22 08:59:07 PDT
Comment on attachment 96297 [details] [diff] [review]
Basic XBL-based patch

brade@netscape.com commented on irc so here is some explaination:
> this.inputField.setSelectionRange(0, 0);
This stops d&d on a non-focused field which is annoys me because I can't see
what the selection is when then field isn't focused.
> if (!/Linux/.test(navigator.platform) &&
I put this test in quickly because apparently a .select() sets the clipboard in
Linux, but I'm not claiming that it's perfect code :-)
Comment 105 Kathleen Brade 2002-08-22 09:41:09 PDT
clarification: I was asking why we check for Linux before checking
this.mIgnoreFocus.  For example, why not:
+            if (!this.mIgnoreFocus && !/Linux/.test(navigator.platform))
Comment 106 neil@parkwaycc.co.uk 2002-08-22 09:51:15 PDT
I'm not claiming perfect code (for once :-)
You might want to have a field so you can change the setting at run time:
<field name="autoSelect">!/Linux/.test(navigator.appName)</field>
And you'd probably want this in C++ to affect all users of <html:input> anyway.
Comment 107 Charles Manske 2002-08-22 09:52:17 PDT
Can we please also fix this for editable menulist?
Comment 108 Charles Manske 2002-08-22 09:58:43 PDT
Wait a minute. Doesn't this patch do exactly the wrong thing, i.e., select 
everything when mouse is clicked inside input? It is supposed to select when
receiving focus by tabbing into it, but NOT when you click in the input widget.
Comment 109 Christopher Blizzard (:blizzard) 2002-08-22 11:31:28 PDT
Recent versions of gtk on linux do select all the text when you give focus to a
widget.
Comment 110 Christian :Biesinger (don't email me, ping me on IRC) 2002-08-22 13:29:06 PDT
Do they also copy the text to the SELECTION buffer? (or whatever the buffer is
called that gets pasted on middle-click)
Comment 111 Akkana Peck 2002-08-22 14:42:00 PDT
See comment 97, though: it's turnaffable in gtk2, it's not the default in the
gtk shipped in any currently shipping distro (i.e. real users haven't seen this
behavior yet), and it's only for tabbing in, not clicking.
Comment 112 ruth@innocent.com 2002-08-23 01:21:17 PDT
"whatever the buffer is called"

You are thinking of the PRIMARY selection and it isn't a buffer, no data is sent
anywhere until you *paste* a selection. The other popular selection is the
CLIPBOARD selection which is used for the metaphor of copying / cutting /
pasting to and from an invisible clipboard.

PRIMARY is automatically asserted by the GTK+ code that deals with text
selections, so I would guess (not having ever used a GTK+2 installation where
this is enabled) that unless someone thought about this and went out of their
way to avoid it, the PRIMARY selection is changed as soon as the text appears
"selected" in the widget.
Comment 113 neil@parkwaycc.co.uk 2002-08-23 01:34:47 PDT
> Wait a minute. Doesn't this patch do exactly the wrong thing, i.e., select 
> everything when mouse is clicked inside input? It is supposed to select when
> receiving focus by tabbing into it, but NOT when you click in the input widget.
No, the mousedown event sets the mIgnoreFocus flag so that the subsequent focus
event gets ignored. Similar code can be seen in navigator.js to handle the URL
bar (although that case has extra code to handle the click selects all pref).

Just a thought: does this have to be done in C++, or will it in fact work in
platformHTMLBindings.xml?
Comment 114 neil@parkwaycc.co.uk 2002-08-23 01:54:33 PDT
Created attachment 96422 [details] [diff] [review]
Proposed patch

Patch to platformHTMLBindings.xml (Windows only, becuase that's my OS) which
fixes it for web pages, textboxes and editable menulists. I'll leave it to the
owners of the different OSes to implement it if they so desire.
Comment 115 Boris Zbarsky [:bz] 2002-09-24 14:31:54 PDT
*** Bug 170576 has been marked as a duplicate of this bug. ***
Comment 116 Aaron Leventhal 2002-09-30 18:09:45 PDT
Neil, your patch seems to work really well, for both HTML and XUL. 
* It doesn't select text in text area, which is good. 
* It selects text in textfields and XUL editable combo boxes, which is good. 
* It doesn't select text on mouse click to focus, which is good.

I can't find anything wrong it -- however, I once heard hyatt say that the
htmlbindings files shouldn't have any javascript in them -- they should only
have command="foo". He said it would cause performance problems.

Hyatt, could you elucidate?
Comment 117 David Hyatt 2002-10-01 15:40:57 PDT
You can't add <implementation>s to common input fields without messing up the 
prototype chain in JS.  I've taken a policy of avoiding using <implementation> 
with any standard HTML elements in order to keep their prototype chains 
unaffected.
Comment 118 neil@parkwaycc.co.uk 2002-10-02 08:32:37 PDT
Another problem with my patch and Pete Zha's patch: focus events caused by
window activate or (over-eager?) use of .focus() also select all. So it looks as
if we still need bug 83496 fixed first.
Comment 119 neil@parkwaycc.co.uk 2002-10-02 08:50:17 PDT
Can we leverage MoveCaretToFocus to do this?
Comment 120 Aaron Leventhal 2002-10-02 14:22:56 PDT
Created attachment 101443 [details] [diff] [review]
Uses esm:MoveCaretToFocus to check for textfield/password controls and select them

This gets everything correct that I know has been mentioned:
* It doesnt' select textareas
* It does select text in editable XUL comboboxes, such as used in the form
manager
* It doesn't select text from mouse clicks.
* It takes affect when a script focuses the textfield, or the user presses a
key (tab or accesskey) to get there.
* It selects text without putting it in the clipboard (because nsAutoCopy.cpp's
selection listener gets reason == nsISelectionListener::NO_REASON, and exits
early)
* It does the right thing for each platform. I used nsILookAndFeel for this,
instead of prefs because gtk2. I filed bug 172203 for this.
Comment 121 Aaron Leventhal 2002-10-02 14:28:41 PDT
Seeking r=/sr=

Neil, thanks for the idea to leverege MoveCaretToFocus -- that worked great.
Comment 122 Aaron Leventhal 2002-10-02 18:23:01 PDT
taking
Comment 123 Aaron Leventhal 2002-10-02 19:10:34 PDT
Created attachment 101484 [details] [diff] [review]
Same thing, but with one more null check on mCurrentFocus

Still seeking r=/sr=
Comment 124 neil@parkwaycc.co.uk 2002-10-03 10:06:54 PDT
Comment on attachment 101484 [details] [diff] [review]
Same thing, but with one more null check on mCurrentFocus

>+  // First, select text fields when focused via keyboard (tab or accesskey)
>+  if (sTextfieldSelectModel == eTextfieldSelect_auto && 
>+      mCurrentFocus &&
>+      mCurrentFocus->IsContentOfType(nsIContent::eHTML_FORM_CONTROL)) {
>+    nsCOMPtr<nsIFormControl> formControl(do_QueryInterface(mCurrentFocus));
>+    PRInt32 controlType;
>+    formControl->GetType(&controlType);
>+    if (controlType == NS_FORM_INPUT_TEXT ||
>+        controlType == NS_FORM_INPUT_PASSWORD) {
>+      nsCOMPtr<nsIDOMHTMLInputElement> inputElement = 
>+        do_QueryInterface(mCurrentFocus);
>+      if (inputElement) {
>+        inputElement->Select();
>+      }
>+    }
>+  }
>+
I'm not a C++ guru so I was just wondering whether you're being careful or you
can't just try QI to nsIDOMHTMLInputElement and call select regardless?
Comment 125 Aaron Leventhal 2002-10-03 10:17:43 PDT
Neil, that would work -- but this is more performant, because QI's are more
expensive than a getter. In most cases the QI will fail, and a failing QI can be
somewhat expensive.
Comment 126 Kathleen Brade 2002-10-03 12:15:54 PDT
Comment on attachment 101484 [details] [diff] [review]
Same thing, but with one more null check on mCurrentFocus

r=brade but please switch Mac and Cocoa to do the selecting also
Comment 127 Aaron Leventhal 2002-10-03 12:18:50 PDT
Created attachment 101562 [details] [diff] [review]
New patch with brade's changes

Seeking sr=
Comment 128 Aaron Leventhal 2002-10-03 12:19:49 PDT
Comment on attachment 101562 [details] [diff] [review]
New patch with brade's changes

Carrying r= forward
Comment 129 Aaron Leventhal 2002-10-03 12:23:55 PDT
Created attachment 101564 [details] [diff] [review]
Oops, forgot to change PRInt32 -> PRInt8 for static in both places
Comment 130 Simon Fraser 2002-10-03 12:37:09 PDT
Please comment in the various nsLookAndFeel impls on what the mysterious 1 and 0
values mean, referring the reader to the nsTextfieldSelectModel enum in
nsEventStateManager.h.
Comment 131 Aaron Leventhal 2002-10-03 12:53:44 PDT
Created attachment 101567 [details] [diff] [review]
Comments added to nsLookAndFeel.cpp, explaining what the mysterious values are
Comment 132 Aaron Leventhal 2002-10-03 13:12:29 PDT
Created attachment 101571 [details] [diff] [review]
Better comments
Comment 133 Aaron Leventhal 2002-10-03 13:17:31 PDT
Created attachment 101573 [details] [diff] [review]
Better comments
Comment 134 Simon Fraser 2002-10-03 15:03:31 PDT
Comment on attachment 101573 [details] [diff] [review]
Better comments

sr=sfraser
Comment 135 neil@parkwaycc.co.uk 2002-10-04 09:27:57 PDT
Comment on attachment 101573 [details] [diff] [review]
Better comments

>+    nsCOMPtr<nsIFormControl> formControl(do_QueryInterface(mCurrentFocus));
>+    PRInt32 controlType;
>+    formControl->GetType(&controlType);
>+    if (controlType == NS_FORM_INPUT_TEXT ||
>+        controlType == NS_FORM_INPUT_PASSWORD) {
>+      nsCOMPtr<nsIDOMHTMLInputElement> inputElement = 
>+        do_QueryInterface(mCurrentFocus);
>+      if (inputElement) {
>+        inputElement->Select();
>+      }
>+    }

I just thought that you could cut it down from 2 to 1 QIs, I don't understand
the benefit in the "extra" QI to nsIFormControl to check the control type.
Comment 136 Aaron Leventhal 2002-10-05 08:56:05 PDT
checked in
Comment 137 Simon Fraser 2002-11-07 16:12:48 PST
This patch has been checked into the CHIMERA_M1_0_1_BRANCH also.
Comment 138 Michael Lefevre 2003-04-27 08:38:16 PDT
verified - tried a number of cases (current win2k nightly) and they all seem to
work as desired.

Note You need to log in before you can comment on or make changes to this bug.