276892 - Selection becomes sticky when selecting text and then releasing mouse button over iframe

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Martijn Wargers (dead)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20041227 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20041227 Firefox/1.0+

See the testcase on how to trigger the bug.

Reproducible: Always

Steps to Reproduce:
1. Hold left mouse button down somewhere in this text
2. Move your mouse into the iframe, while holding the mouse button down
3. Release your mouse button.

Actual Results:  
Selection becomes 'sticky', e.g. the selection follows your mouse movement when
it should not.

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Sounds like a widget/capturing issue...

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

First of all, when the mouse button is released we should just unconditionally
stop capturing. Turns out we don't actually need IsMouseCaptured in that case.

Secondly, when the mouse button is released we need to tell the selection
controller that belongs to the document where capturing started. This patch
continues to tell the current document's selection controller as well, so as
not to disturb the special processing there (which I don't fully understand).

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'll try to look tonight, but why is it ok to capture the mouse if it's already
captured (in HandlePress)?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why wouldn't it be OK? We should never be capturing the mouse when the button is
up, anyway.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173586 [details] [diff] [review]
fix

If we could easily add asserts on the capture state, that would be good... if
not, don't bother.

Also, two more questions:

1)  What if the user mouses down in the mozilla window and then moves the mouse
out of it and releases outside?  Won't we get into a state where the mouse is
up but we're still capturing?

2)  Why don't we need to check the DisplaySelection() type on the prescontext
of activeFrame?

r+sr=bzbarsky with those addressed.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #6)
> (From update of attachment 173586 [details] [diff] [review] [edit])
> If we could easily add asserts on the capture state, that would be good... if
> not, don't bother.

OK, I'll inline IsMouseCaptured into an #ifdef DEBUG block where we call it from
nsBlockFrame, and assert there that IsMouseCaptured is false.

> Also, two more questions:
> 
> 1)  What if the user mouses down in the mozilla window and then moves the
> mouse out of it and releases outside?  Won't we get into a state where the
> mouse is up but we're still capturing?

No, because we're capturing so we should see the mouse release :-).

> 2)  Why don't we need to check the DisplaySelection() type on the prescontext
> of activeFrame?

We should, I'll put it back in.

Comment 8

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #7)
> OK, I'll inline IsMouseCaptured into an #ifdef DEBUG block where we call it
> from nsBlockFrame, and assert there that IsMouseCaptured is false.

Will that assert fire if the user mouses down outside the Mozilla window and
then mouses up inside it?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #8)
> (In reply to comment #7)
> > OK, I'll inline IsMouseCaptured into an #ifdef DEBUG block where we call it
> > from nsBlockFrame, and assert there that IsMouseCaptured is false.
> 
> Will that assert fire if the user mouses down outside the Mozilla window and
> then mouses up inside it?

No. We're asserting that on a mousedown event, we were not previously capturing
the mouse.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Good.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

grrr. Looks like we do need to test for mouse capturing somehow because the call
to nsFrame::HandleEvent might capture the mouse and we shouldn't override it.
I'll make a new patch.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: more conservative fix

This fix is more conservative. I'm leaving the IsMouseCaptured calls in and
just doing the minimum necessary to fix this bug. It's still OK to
unconditionally drop capture in HandleRelease since we still should never be
holding capture after mouse-up. (I wish toolkits just always did capture
themselves, the policy is easy: start capturing to window on mousedown, stop on
mouse release.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176507 [details] [diff] [review]
more conservative fix

>+      NS_STATIC_CAST(nsFrame*, activeFrame)->DisplaySelection(activeFrame->GetPresContext())

We could avoid this cast if DisplaySelection() were a static method taking
nsIFrame* as the first arg, I think... but either way, r+sr=bzbarsky.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in