Closed Bug 276892 Opened 20 years ago Closed 19 years ago

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

Categories

(Core :: DOM: Selection, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: roc)

References

Details

(Keywords: testcase)

Attachments

(2 files, 1 obsolete file)

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.
Keywords: testcase
Attached file Testcase
Sounds like a widget/capturing issue...
Attached patch fix (obsolete) — Splinter Review
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).
Assignee: selection → roc
Status: NEW → ASSIGNED
Attachment #173586 - Flags: superreview?(bzbarsky)
Attachment #173586 - Flags: review?(bzbarsky)
I'll try to look tonight, but why is it ok to capture the mouse if it's already
captured (in HandlePress)?
Why wouldn't it be OK? We should never be capturing the mouse when the button is
up, anyway.
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.
Attachment #173586 - Flags: superreview?(bzbarsky)
Attachment #173586 - Flags: superreview+
Attachment #173586 - Flags: review?(bzbarsky)
Attachment #173586 - Flags: review+
(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.
(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?
(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.
Ah, ok.  Good.
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.
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.)
Attachment #173586 - Attachment is obsolete: true
Attachment #176507 - Flags: superreview?(bzbarsky)
Attachment #176507 - Flags: review?(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.
Attachment #176507 - Flags: superreview?(bzbarsky)
Attachment #176507 - Flags: superreview+
Attachment #176507 - Flags: review?(bzbarsky)
Attachment #176507 - Flags: review+
checked in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Depends on: 336592
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: