Closed Bug 976673 Opened 6 years ago Closed 6 years ago

While mouseup event is being dispatched, event handlers cannot steal focus from inaccessible element if preceding mousedown event's preventDefault() is called

Categories

(Core :: DOM: Core & HTML, defect)

2.0 Branch
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox29 --- affected
firefox30 --- affected
firefox31 --- affected
firefox32 --- affected
firefox33 --- fixed
firefox-esr24 --- affected

People

(Reporter: BenWa, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

Attached image caret.gif
STR:
1) Open http://jsfiddle.net/fNPvf/2446/
2) Select the input box
3) Select each of the other boxes (HTML, CSS, JS)
4) You should now have several blinking carets.
Reproducible on release FF 26.0
And current release 27.0.1
Sigh, I was hoping that this is a regression. :(
Still worth trying it out on older revisions. This should fail early if it isn't.
-g 2009-12-10-03-mozilla-central-firefox-3.7a1pre.en-US.linux-x86_64
-g 2009-12-11-03-mozilla-central-firefox-3.7a1pre.en-US.linux-x86_64 9af2a428dcb1
-b 2009-12-12-03-mozilla-central-firefox-3.7a1pre.en-US.linux-x86_64 d379a17cbf8f
-b 2009-12-13-03-mozilla-central-firefox-3.7a1pre.en-US.linux-x86_64

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9af2a428dcb1&tochange=d379a17cbf8f
Keywords: regression
Summary: 4 caret on jsfiddle → 4 carets on jsfiddle
OS: Mac OS X → All
Version: Trunk → 2.0 Branch
(In reply to [:Aleksej] from comment #5)
> -g 2009-12-10-03-mozilla-central-firefox-3.7a1pre.en-US.linux-x86_64
> -g 2009-12-11-03-mozilla-central-firefox-3.7a1pre.en-US.linux-x86_64
> 9af2a428dcb1
> -b 2009-12-12-03-mozilla-central-firefox-3.7a1pre.en-US.linux-x86_64
> d379a17cbf8f
> -b 2009-12-13-03-mozilla-central-firefox-3.7a1pre.en-US.linux-x86_64
> 
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=9af2a428dcb1&tochange=d379a17cbf8f

Thanks, Aleksej!  It would be nice if someone could please bisect this range, none of those patches jump out at me as the culprit after a quick look.
In local build
Last Good: ade8ba415789
First Bad: a8f46161c891

Regressed by:a8f46161c891	Masayuki Nakano — Bug 125282 Webpage-JS can steal focus from URLbar / chrome r=enndeakin
Blocks: 125282
Component: Selection → DOM: Core & HTML
Huh, interesting!  Perhaps that bug changed things in a way that the editor is no longer notified when the field gets blurred out in order to hide the caret?  Masayuki, can you please take a look?  Thanks!
Flags: needinfo?(masayuki)
Sure. It's odd. <input> element doesn't receive blur event...
http://jsfiddle.net/d_toybox/89JwW/
Flags: needinfo?(masayuki)
Attached patch part.1 Add tests (obsolete) — Splinter Review
Hmm, this is very complicated.

First, the test page is under different domain. Therefore, other editors in parent document cannot steal focus from the test page.

The page prevents the default of mosuedown event. Therefore, ESM don't move focus to its parent document at mousedown event. However, the parent document tries to steal focus at mouseup event. Then, it fails.

We need to investigate other browsers' behavior before writing a patch.
This must be another case of bug 591890.
Blocks: 591890
Attached patch part.1 Add tests (obsolete) — Splinter Review
Oops, forgot to add the new files.
Attachment #8429042 - Attachment is obsolete: true
http://jsfiddle.net/89JwW/3/
http://jsfiddle.net/89JwW/4/

Looks like that a call of preventDefault() of mosuedown doesn't allow to move focus.

However, from mouseup event handler, it allows to move focus both on Chrome and IE.
Status: NEW → ASSIGNED
Assignee: nobody → masayuki
Attached patch part.1 Add testsSplinter Review
Attachment #8429045 - Attachment is obsolete: true
Attachment #8429845 - Flags: review?(enndeakin)
This patch allows event handlers which run both dispatching mousedown and mouseup events to steal focus from any node.

This causes a lot of oragnes, though.
Attachment #8429848 - Flags: review?(enndeakin)
This fixes most new oranges which is caused by unexpected NS_ASSERTION() calls.

The reason is, those tests synthesize another mouse events from mouse event handler. This means that input event is nested on the stack. This is unusual path and not testing actual behavior.

Therefore, this patch uses SimpleTest.executeSoon() for synthesizing another mouse or key events.

FYI: The hitting assertion is

NS_ASSERTION(!aDocument || !mMouseButtonEventHandlingDocument,
             "Some mouse button down events are nested?");

in nsFocusManager::SetMouseButtonHandlingDocument().
Attachment #8429852 - Flags: review?(bugs)
Even if we land the patch part.3, there is some oranges.

That is caused by a mistake of the design of nsFocusManager::SetMouseButtonHandlingDocument(). It doesn't allow nested mouse button events. However, it occurs with modal dialog if it's opened from mouse button event handler. Therefore, we should allow to nested mouse button event.

This patch makes the method returns old document. And AutoHandlingUserInputStatePusher stores it and restores the document at destroying.

However, there is a concern. By this change, we will lose a way to detect nested input events which is typically caused by bad automated tests like fixed by part.3. But this is out of scope of this bug.
Attachment #8429859 - Flags: review?(enndeakin)
Attachment #8429859 - Flags: review?(bugs)
Summary: 4 carets on jsfiddle → While mouseup event is being dispatched, event handlers cannot steal focus from inaccessible element if preceding mousedown event's preventDefault() is called
Attachment #8429848 - Flags: review?(enndeakin) → review+
Attachment #8429859 - Flags: review?(enndeakin) → review+
Attachment #8429852 - Flags: review?(bugs) → review+
Attachment #8429859 - Flags: review?(bugs) → review+
Thank you for your review!

Enn: Don't you forget to review the part.1 patch? You did only 2 patches of 3 requests.
Flags: needinfo?(enndeakin)
No I didn't forget. I needed more time to understand it.
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #19)
> No I didn't forget. I needed more time to understand it.

Okay, thank you.
(In reply to Neil Deakin from comment #19)
> No I didn't forget. I needed more time to understand it.

FYI: The test tests whether the parent document's script can steal focus from the iframe's document which is inaccessible from parent due to different domain.

The test case is, at mousedown event handler prevents it's default. This causes moving focus to the mousedowned document.
http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp#2698
http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp#2816

The message event is used for initializing and checking the result from the parent document to the iframe document.
> This causes moving focus to the mousedowned document.

I meant this causes preventing to move focus to the mousedowned document.
Attachment #8429845 - Flags: review?(enndeakin) → review+
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.