70519 - Select All (+ Copy) does not clobber the X primary selection

Status: RESOLVED FIXED

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

Description

[Jeffrey Baker]

If you select the contents of a page by drag selecting, the X cut buffer is
filled with the contents of the selection.  But, if you select the same contents
using "Select All", the cut buffer is not replaced.  This results in the
situation where some text is highlighted in Mozilla, but middle-button pasting
into an xterm or whatever pastes something else.

Hitting Ctrl + C to copy has no effect, other than the desired one of replacing
the X clipboard.

Observed on 2001-02-28-08

Comment 1

[Boris Zbarsky [:bzbarsky]]

updating summary.  Cut buffers are not the same thing as the clipboard in X and
are a totally different mozilla bug

Comment 2

[Jeffrey Baker]

Updating bzbarsky's pendantry.  I am not talking about the clipboard, which
works.  I am talking about the X primary selection, which is supposed to be
whatever is highlighted.  The bug is that I can highlight in Mozilla via "Select
All" and the primary selection will remain unchanged.

Comment 3

[rubydoo123]

reassign to anthonyd, setting to moz1.0

Comment 4

[anthonyd]

i have no clue about this bug, so im cc'ing kin on this, maybe he can drop some 
knowledge on me , WERD!

anthonyd

Comment 5

[Jeffrey Baker]

I retested this and it still doesn't work in 0.9.1.  Maybe I can fix it, if I
find where Select All hooks in.

Comment 6

[Jeffrey Baker]

Okay I hacked around, didn't come up with a fix, but possibly cornered the
problem.  The bit of code is in nsAutoCopyService::NotifySelectionChanged(),
which expects to get aReason why the selection changed.  If the reason isn't
MOUSEUP_REASON, the clipboard isn't touched.  Unfortunately the "Select All"
patch goes through DocumentViewerImpl::SelectAll(), and that class doesn't
understand how to set the reason before calling nsTypedSelection::AddRange().

So, two things need to happen: 

1) all paths that change the selection need to set the reason for the change. 
This includes DocumentViewer and nsTextHelper and probably some other places.

2) nsAutoCopyService needs to change the clipboard for more events than just
mouseup.

I leave the implementation of these fixes in your capable hands :)

Comment 7

[Jeffrey Baker]

Attachment: Proposed patch

Comment 8

[Jeffrey Baker]

Attachment: Proposed patch, this time with all files

Comment 9

[Jeffrey Baker]

Sorry for the noise, you can ignore the first blown patch.  The propsed patch is
against the trunk, and it fixes several selection problems very simply.  First,
it makes nsDocumentViewer::SelectAll use a nsTypedSelection::SelectAllChildren,
which is a method that knows how to properly set the mReason member.  On the
other end, it changes AutoCopyService to react to more event types.  This patch
builds and fixes the following cases:

1) Select All in the content area
2) Select All in a text input
3) Select All in a textarea
4) Select All in the URL bar

CC'ing the person whose method I propose to change, and reassigning to myself. 
Someone review me.

Comment 10

[Jeffrey Baker]

Are any of the people CCd here the right person for review?  If not, I'd be
grateful to know from whom I should solicit review.

Comment 11

[kinmoz]

One thing I'm concerned about with this change is that textwidgets use 
SelectAll() when replacing their contents ... see 
nsGfxTextControlFrame2::SetTextControlFrameState()

This method gets called whenever someone sets the value of a textwidget via the 
DOM/JS.

Can you verify that your change doesn't cause accidental XClipboard overriding? 
You can do this by going to several websites and hitting back and forward on the 
browser window. Each time you press back and forward, the browser 
programatically sets the value of the URL bar textfield ... if my hunch is 
correct, you might see that the XClipboard contents change every time the page 
changes.

Cc'ing more XHeads [e-mail], [e-mail] and 
[e-mail].

Comment 12

[Jeffrey Baker]

kin, I tried your test and did not have the problem you predicted.  The URL bar
changed as expected, and the primary selection contents did not change.

Comment 13

[Jesse Ruderman]

*** Bug 89868 has been marked as a duplicate of this bug. ***

Comment 14

[timeless]

Can someone please explain (better, provide a specification) what is being 
asked for and how QA could possibly verify this bug as fixed when some code is 
committed?

My understanding was that we intentionally allowed the keyboard to _not_ 
clobber the clipboard unless you used commands to copy/cut the mozilla 
selection.  If we change this behavior, then people will yell and scream when 
ctrl-l (focus location bar) kills their clipboard.

Here is the behavior i currently expect (mozilla-xlib; xp-keybindings)
1. left click into a text area. Clipboard is cleared (ownership goes to 
mozilla).
2. drag to select some text. clipboard is owned by mozilla and pasting pastes 
the currently highlighted selection.
3. select a url in lynx. Clipboard ownership is given to the xterm app, content 
is the selected url.
4. middle click content area of mozilla. New window opens w/ url from 
lynx/sirc.
5. select url from sirc. Clipboard ownership is given to the xterm app, content 
is the selected url.
6. sloppy focus mozilla window2.
7. control-l. one of the following:
a. cursor goes to beginning of location bar, and url isn't selected
b. focus goes to location bar and url is selected.
7b. shift-end. Location bar area is selected.
8. middle click or ctrl-v. url from xterm is pasted.
9. control-z (or edit>undo). old url is restored. old url is selected.
10. middle click content.  url from sirc is opened in a new window.
11. sloppy focus window2. (urlbar gets focus)
12. ctrl-c. mozilla captures clipboard; url from lynx aka old url which is in 
location bar is made available to clipboard.

if this isn't what happens now, please correct me (my *nix build env is 
currently unusable).

Comment 15

[Jeffrey Baker]

How do you expect anyone to remember all that inconsistent behavior?  Anytime
text is highlighted, it should be the primary selection.  Any input area should
accept pasting from the primary selection.  Clicking in an input area should
never clear the primary selection.

The current behavior is inconsistent.  Everytime text is highlighted, including
ctrl-l to focus the location bar, the primary selection changed to the
highlighted text.  The ONLY exception is select-all which doesn't touch the
selection.

Also *please* do not conflate the PRIMARY and the CLIPBOARD

Comment 16

[Anthony DeRobertis]

Jeffry, at present control-L does *not* assert ownership of the primary
selection. Try it. At least in 0.9.2 it does not.

Comment 17

[Jeffrey Baker]

You are right Anthony.  That is a difference between 0.9.1 and 0.9.2.  I
previously tested the former.

My point is still that an X user expects whatever is highlighted to become the
primary selection.

I just noticed that selecting text using the arrow keys in an input or textarea
also does not replace the primary selection. whereas selecting the same text
with the mouse does replace the primary.  Perhaps this bug should be changed to
"X primary selection handling needs to be wrung out".

Comment 18

[Boris Zbarsky [:bzbarsky]]

*** Bug 94362 has been marked as a duplicate of this bug. ***

Comment 19

[Keyser Sose]

*** Bug 84217 has been marked as a duplicate of this bug. ***

Comment 20

[lchiang]

Does this need to be moved to 0.9.5 since 0.9.4 is done? per PDT

Comment 21

[Jeffrey Baker]

if (traction == 0)
    milestone++;

Comment 22

[sairuh (rarely reading bugmail)]

gently prodding kin and mike [or, whoever's appropriately qualified] for r/sr
for this puppy... :)

Comment 23

[Akkana Peck]

Comment on attachment 39458 [details] [diff] [review]
Proposed patch, this time with all files

It would be nice if someone associated with selection looked at this patch, but it looks good to me, and works for me (select-all and key-initiated selections now go into the primary selection as I would expect).  Accel-L still does not copy to the primary buffer, which I believe is as intended and is probably the best way to go. r=akkana.

Comment 24

[kinmoz]

Comment on attachment 39458 [details] [diff] [review]
Proposed patch, this time with all files

You can probably remove this too:


>   rv = selection->RemoveAllRanges();
>   if (NS_FAILED(rv)) return rv;


since your call to:


>+  rv = selection->SelectAllChildren(bodyNode);


should remove all ranges anyways.

sr=[e-mail] ... with the assumption that programatically setting
the contents of a text widget via JS (which uses SelectAll()) still doesn't
override the main clipboard.

Comment 25

[Jeffrey Baker]

I'm going to rebuild and test this patch versus JS input widget frobbing
tonight.  If it works I'll get #mozilla to commit it.

Comment 26

[Jeffrey Baker]

Attachment: Updated patch that applies with less fuzz

Comment 27

[Jeffrey Baker]

It's the same patch but the other one had rotted and wouldn't apply.  I tested
this with some JS code which replaced and edits the contents of <input> and it
did not clobber the selection.

Comment 28

[Jeffrey Baker]

checked in