clipboard.autocopy does not work properly, selected text is no longer available in clipboard (or rather in PRIMARY)

RESOLVED WORKSFORME

Status

()

Core
Selection
--
major
RESOLVED WORKSFORME
7 years ago
7 years ago

People

(Reporter: Alice0775 White, Assigned: masayuki)

Tracking

(Depends on: 2 bugs, {regression})

Trunk
mozilla9
All
Linux
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

7 years ago
Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/6df31af4cca6
Mozilla/5.0 (X11; Linux i686; rv:8.0a1) Gecko/20110721 Firefox/8.0a1 ID:20110721030828

middlemouse paste is broken.
See http://forums.mozillazine.org/viewtopic.php?p=11048651#p11048651

Reproducible: Always

Steps to Reproduce:

1. Start Firefox with clean profile.
2. Open any page
3. Select text
4. Middle click on Search Bar

Actual Results: 
  Nothing happens or previous copied text is pasted

Expected Results: 
  Selected text should be pasted

Regression window(cahed m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/5c7a49dfa7f7
Mozilla/5.0 (X11; Linux i686; rv:8.0a1) Gecko/20110720 Firefox/8.0a1 ID:20110720133756
Fails:
http://hg.mozilla.org/mozilla-central/rev/be5295604fd0
Mozilla/5.0 (X11; Linux i686; rv:8.0a1) Gecko/20110720 Firefox/8.0a1 ID:20110720173300
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5c7a49dfa7f7&tochange=be5295604fd0

Regressed by:
be5295604fd0	Masayuki Nakano — Bug 671319 Should abort drag for selection at resetting capture r=smaug
Is the behavior different on Windows and Linux? I cannot copy by the selected text by middle click on Windows (i.e., text in clipboard already is always pasted).
(Reporter)

Comment 2

7 years ago
(In reply to comment #1)
> Is the behavior different on Windows and Linux? I cannot copy by the
> selected text by middle click on Windows (i.e., text in clipboard already is
> always pasted).
Yes,It is different between Windows and Linux(X Window System).
Looks like the function can be enabled by clipboard.autocopy. However, I cannot enable this feature on Windows, it's odd...
Okay, I see the cause. But I'm not sure why the patch caused this, I'll investigate it more.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED

Updated

7 years ago
Duplicate of this bug: 673173

Comment 6

7 years ago
The subject of this bug is currently slightly misleading.

The problem is not pasting. If you select something from another program there is no problem to paste it, using the middle mouse button, into the latest nightly.

But if you select something with the mouse in the latest nightly it doesn't get copied to the copy/paste buffer and you can't paste it, neither into the latest nightly nor into another program.
(Reporter)

Updated

7 years ago
Summary: middlemouse paste does not work properly → clipboard.autocopy does not work properly, selected text is no longer available in clipboard
Severity: normal → major
Summary: clipboard.autocopy does not work properly, selected text is no longer available in clipboard → clipboard.autocopy does not work properly, selected text is no longer available in clipboard (or rather in PRIMARY)

Comment 7

7 years ago
I see this in Thunderbird trunk (Shredder) too.

  Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110722 Thunderbird/8.0a1

Comment 8

7 years ago
(In reply to comment #1)
> Is the behavior different on Windows and Linux? I cannot copy by the
> selected text by middle click on Windows (i.e., text in clipboard already is
> always pasted).

Just to clarify what's going on here, X has two clipboards (well technically three, but nobody uses one of them for anything) PRIMARY, and CLIPBOARD. The bug is that selected text is not going into PRIMARY as it ought to.
Duplicate of this bug: 673585
Hardware: x86 → All
Created attachment 547870 [details] [diff] [review]
Patch part.1 selection change reason must be MOUSEUP_REASON when dragging for selection finishes normally

There are two bugs, this patch fixes the first one.

This is a simple mistake. I forgot to change nsFrame::HandleRelease(). By the bug 671319 change, we must call nsFrameSelection::SetMouseDownState(PR_FALSE) before nsIPresShell::SetCapturingContent() in nsFrame::HandleRelease(). If we do so, the selection change reason becomes MOUSEUP_REASON. Otherwise (i.e., currently), nsFrameSelection thinks the drag is aborted rather than finished normally. Then, the selection change reason is NO_REASON.

In PresShell::HandleEventInternal(), we're releasing capturing content always.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7097
So, we don't need to release it in nsFrame::HandleRelease(), therefore, this patch just removes it.

And the nsFrame::HandleRelease()'s only one caller at non-mouseup reason is nsSliderFrame::HandleEvent() when it's called for NS_MOUSE_EXIT_SYNTH. This needs to call nsIPresShell::SetCapturingContent(nsnull, 0) manually. However, I think that this is wrong behavior as I wrote the comment in this patch. I'll look for such bug.
Attachment #547870 - Flags: review?(Olli.Pettay)
Created attachment 547872 [details] [diff] [review]
Patch part.2 add new selection reason for drag for selection aborted

The other cause is fixed by this patch.

Now, we're using NO_REASON when aborted a drag for selection. However, we should use new reason at that time. For example,
> data:text/html,<script type="text/javascript">var count = 0;</script><input value="aaaaaaaaaaaaaaaaaaaaaaa" onmousedown="count=0;" onmousemove="if (count++ == 50) document.releaseCapture();" onselect="alert('onselect fired');">

load this url, and mousedown in the <input> and select a character and release it. Then, you could see an alert dialog if you applied the part.1 patch.

However, if you continued to mousemove, the drag would be aborted automatically but you wouldn't see the alert dialog at that time.

I define a new reason DRAG_ABORTED_REASON. And I change each listeners which are checking the reason.
Attachment #547872 - Flags: review?(Olli.Pettay)
Created attachment 547876 [details] [diff] [review]
Patch part.2 add new selection reason for drag for selection aborted
Attachment #547872 - Attachment is obsolete: true
Attachment #547872 - Flags: review?(Olli.Pettay)
Attachment #547876 - Flags: review?(Olli.Pettay)
Comment on attachment 547876 [details] [diff] [review]
Patch part.2 add new selection reason for drag for selection aborted

This is becoming very complex. Is there really no other way to fix this problem?
Or is there some other, less regression risky way to fix bug 671319?
(In reply to comment #13)
> This is becoming very complex. Is there really no other way to fix this
> problem?

I think that if we can use MOUSEUP_REASON in nsFrameSelection::AbortDragForSelection(), there is no problem. However, it's very strange by the name. If we don't mind this, it's most simple fix and not risky because some other points are calling nsFrameSelection::SetMouseDownState(PR_FALSE) at non-mouseup situation. For example:

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#732
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#6467
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#1124

I'm not sure whether there are some cases actually to need to distinguish MOUSEUP_REASON and DRAG_ABORTED_REASON. My another idea is, we can define DRAG_ABORTED_REASON as 0x24. Then, (aReason & MOUSEUP_REASON) hits DRAG_ABORTED_REASON too.

> Or is there some other, less regression risky way to fix bug 671319?

This cause isn't bug 671319 actually, I think it's just a trigger. Before bug 552707, we didn't abort dragging for selection when mouse capture is released forcibly. By the behavior, I can see some strange behavior -- I have released mouse button but I'm still dragging. So, the older versions didn't think that dragging is never aborted. This wrong logic makes this bug (only part.2) with current code. Therefore, I believe that bug 671319's patch used a right approach.

# and I realized that I shouldn't change nsHTMLObjectResizer. It's MOUSEDOWN_REASON, not MOUSEUP_REASON...
Created attachment 547976 [details] [diff] [review]
Patch part.2 approach #1
Attachment #547876 - Attachment is obsolete: true
Attachment #547876 - Flags: review?(Olli.Pettay)
Attachment #547976 - Flags: review?(Olli.Pettay)
Created attachment 547978 [details] [diff] [review]
Patch part.2 approach #2

This is simplest, but I like #1 better. Please choose from these two (or, is there another approach?).
Attachment #547978 - Flags: review?(Olli.Pettay)
Created attachment 547981 [details] [diff] [review]
Patch part.2 approach #1

This is better than previous #1 patch. If we add MOUSEUP_REASON at runtime, (aReason & DRAG_ABORTED_REASON) doesn't hit to MOUSEUP_REASON.
Attachment #547976 - Attachment is obsolete: true
Attachment #547976 - Flags: review?(Olli.Pettay)
Attachment #547981 - Flags: review?(Olli.Pettay)
Comment on attachment 547978 [details] [diff] [review]
Patch part.2 approach #2

The problem with this is that AbortDragForSelection() is called also in
nsIPresShell::SetCapturingContent which can be called while mouse is still down.
Attachment #547978 - Flags: review?(Olli.Pettay) → review-
Attachment #547870 - Flags: review?(Olli.Pettay) → review+
(In reply to comment #18)
> The problem with this is that AbortDragForSelection() is called also in
> nsIPresShell::SetCapturingContent which can be called while mouse is still
> down.

Of course, yes. Do you think the dragging for selection shouldn't be aborted by releasing the mouse capture? Or, do you think we should deny the releasing mouse capture during dragging for selection?
Comment on attachment 547876 [details] [diff] [review]
Patch part.2 add new selection reason for drag for selection aborted

I and smaug discussed the issues on IRC.

I'm re-requesting review to smaug again.
Attachment #547876 - Attachment is obsolete: false
Attachment #547876 - Flags: review?(Olli.Pettay)
Attachment #547978 - Attachment is obsolete: true
Attachment #547981 - Attachment is obsolete: true
Attachment #547981 - Flags: review?(Olli.Pettay)
Created attachment 547999 [details] [diff] [review]
Patch part.2 add new selection reason for drag for selection aborted
Attachment #547876 - Attachment is obsolete: true
Attachment #547876 - Flags: review?(Olli.Pettay)
Attachment #547999 - Flags: review?(Olli.Pettay)
Attachment #547999 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 547999 [details] [diff] [review]
Patch part.2 add new selection reason for drag for selection aborted

oops, this needs sr for interface change.

But I'm not sure who is a good super reviewer for nsISelectionListener. Smaug who is an expert of event handling was already r+ for this patch. Therefore, I'll request sr to roc.

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/content/base/public/nsISelectionListener.idl&rev=HEAD&mark=1.4
http://hg.mozilla.org/mozilla-central/filelog/5d1799e16fe9/content/base/public/nsISelectionListener.idl
Attachment #547999 - Flags: superreview?(roc)
landed part.1 first:
http://hg.mozilla.org/mozilla-central/rev/de43fbe04808

You cannot see this bug by this landing, however, don't close this bug. The part.2 bug shows by part.1 fix.
Flags: in-testsuite+
Whiteboard: part.1 landed, part.2 needs sr
Target Milestone: --- → mozilla8
(In reply to comment #23)
> You cannot see this bug by this landing, however, don't close this bug. The
> part.2 bug shows by part.1 fix.

It means that you can still see this bug if drag is aborted by something. It'll be fixed by part.2.
Attachment #547999 - Flags: superreview?(roc) → superreview+

Updated

7 years ago
Depends on: 675614
Depends on: 675865
The regression causes are backedout temporarily. But they will be back again soon for mozilla9. See bug 675865.

The all works for this bug is already done, therefore, I mark this bug as WORKSFORME temporarily.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WORKSFORME
Whiteboard: part.1 landed, part.2 needs sr
Target Milestone: mozilla8 → mozilla9
You need to log in before you can comment on or make changes to this bug.