[e10s] bugzilla text boxes fail to take focus

RESOLVED FIXED in Firefox 48

Status

()

Core
Event Handling
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: jimm, Assigned: mconley)

Tracking

({regression})

Trunk
mozilla49
regression
Points:
---

Firefox Tracking Flags

(e10sm9+, firefox48 fixed, firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year ago
STR:

1) open any bug and set the needinfo flag to ?

expected: focus should move to the text input
actual: focus isn't set in the text input

This does not reproduce under non-e10s.

Neil, any ideas here?
Flags: needinfo?(enndeakin)

Comment 1

a year ago
Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3e069aea556fa27693c7ea0660f98107148143af&tochange=d2bd7cfc6d8c0aaf1dab44de0e6b8ad5aff13f4f

Regressed by: bug 1266575
If its a regression, let's ask Mike then!
Flags: needinfo?(enndeakin) → needinfo?(mconley)
(Assignee)

Comment 3

a year ago
Investigating.
Blocks: 1266575
Flags: needinfo?(mconley)
(Assignee)

Comment 4

a year ago
Created attachment 8756442 [details]
Manual test case
Assignee: nobody → mconley
(Assignee)

Updated

a year ago
Attachment #8756442 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 5

a year ago
Created attachment 8756445 [details]
MozReview Request: Bug 1275384 - Dispatch mousedown, mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r?felipe

We were using nsIDOMWindowUtils to send mousedown and mouseup events
to the <select> input after a selection was made in e10s mode, but
doing so causes focus to be pulled back to the <select> if any input
or change event handlers tried to shift focus. For example, the
reviewer input on Bugzilla was having its focus stolen after setting
the review flag to r?, which was how this bug was discovered.

Review commit: https://reviewboard.mozilla.org/r/55180/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55180/
Attachment #8756445 - Flags: review?(felipc)
Comment on attachment 8756445 [details]
MozReview Request: Bug 1275384 - Dispatch mousedown, mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r?felipe

The patch looks good but I'd like to ask smaug to take a quick look, just as a way to check if this difference in behavior is surprising or expected
Attachment #8756445 - Flags: feedback?(bugs)
https://reviewboard.mozilla.org/r/55180/#review51898

::: toolkit/modules/SelectContentHelper.jsm:133
(Diff revision 1)
> -                       .getInterface(Ci.nsIDOMWindowUtils);
> -          let rect = this.element.getBoundingClientRect();
> -          dwu.sendMouseEvent("mousedown", rect.left, rect.top, 0, 1, 0, true);
> -          dwu.sendMouseEvent("mouseup", rect.left, rect.top, 0, 1, 0, true);
> +          for (let eventName of MOUSE_EVENTS) {
> +            let mouseEvent = new win.MouseEvent(eventName, {
> +              'view': win,
> +              'bubbles': true,
> +              'cancelable': true,
> +            });

So the coordinates are what?

It is not clear to me why this works but the old code doesn't
(Assignee)

Comment 8

a year ago
(In reply to Olli Pettay [:smaug] from comment #9)
> https://reviewboard.mozilla.org/r/55180/#review51898
> 
> ::: toolkit/modules/SelectContentHelper.jsm:133
> (Diff revision 1)
> > -                       .getInterface(Ci.nsIDOMWindowUtils);
> > -          let rect = this.element.getBoundingClientRect();
> > -          dwu.sendMouseEvent("mousedown", rect.left, rect.top, 0, 1, 0, true);
> > -          dwu.sendMouseEvent("mouseup", rect.left, rect.top, 0, 1, 0, true);
> > +          for (let eventName of MOUSE_EVENTS) {
> > +            let mouseEvent = new win.MouseEvent(eventName, {
> > +              'view': win,
> > +              'bubbles': true,
> > +              'cancelable': true,
> > +            });
> 
> So the coordinates are what?
> 
> It is not clear to me why this works but the old code doesn't

The coordinates are 0,0 for the rect of the <select> element (Chrome seems to do the same thing after a click on an <option>).

This "works" in that it dispatches the expected mouse events, but it doesn't draw focus because we're not using DWU to actually synthesize mouse activity; we're just synthesizing the mouse events instead.

Or am I misunderstanding the question?
Flags: needinfo?(bugs)
Hmm, totally bogus coordinates sounds odd. But ok fine if others do that do.

What about event.buttons?
Flags: needinfo?(bugs)
(Reporter)

Comment 10

a year ago
Tracy, please track down a regression range on this.
Flags: needinfo?(twalker)
(Reporter)

Comment 11

a year ago
(In reply to Jim Mathies [:jimm] from comment #3)
> Tracy, please track down a regression range on this.

Sorry, found comment 1 a little farther down in my mailbox
Flags: needinfo?(twalker)
Keywords: regressionwindow-wanted
Comment on attachment 8756445 [details]
MozReview Request: Bug 1275384 - Dispatch mousedown, mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r?felipe

So I gave feedback. .buttons handling at least looks wrong this way.
But that is fixable from JS. But please go through also other properties on the event and ensure they have useful values.
Attachment #8756445 - Flags: feedback?(bugs)
Comment hidden (obsolete)
(Assignee)

Comment 14

a year ago
Comment on attachment 8756445 [details]
MozReview Request: Bug 1275384 - Dispatch mousedown, mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55180/diff/1-2/
Attachment #8756445 - Attachment description: MozReview Request: Bug 1275384 - Dispatch mousedown, mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r?felipe → MozReview Request: Bug 1275384 - Dispatch mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r?felipe
(Assignee)

Comment 15

a year ago
So it looks like on Windows, Blink fires a mouseup and then a click after an <option> is selected - even if the user uses the keyboard. Oddly, it's different for OS X (no mouseup or click after an option selection), but I'd really rather not emulate that kind of inconsistency, so I'm going to go with the Windows convention.

I did a comparison of each property for the mouseup and click events for both this patch and Blink, and we seem to match all of the ones that are part of the standard. There are some engine-specific or experimental properties on these events that didn't match in some cases, but I'm not counting those.
On linux blink fires mousedown when opening select, and then mouseup and click when selecting something on it.

Does Blink really not have .buttons to set to anything when dispatching click?
(Assignee)

Comment 17

a year ago
(In reply to Olli Pettay [:smaug] from comment #16)
> On linux blink fires mousedown when opening select, and then mouseup and
> click when selecting something on it.
> 
> Does Blink really not have .buttons to set to anything when dispatching
> click?

It has it set to 0, at least in my testing.
(Assignee)

Updated

a year ago
tracking-e10s: ? → m9+
(Assignee)

Comment 18

a year ago
Comment on attachment 8756445 [details]
MozReview Request: Bug 1275384 - Dispatch mousedown, mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55180/diff/2-3/
Attachment #8756445 - Attachment description: MozReview Request: Bug 1275384 - Dispatch mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r?felipe → MozReview Request: Bug 1275384 - Dispatch mousedown, mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r?felipe
Comment on attachment 8756445 [details]
MozReview Request: Bug 1275384 - Dispatch mousedown, mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r?felipe

https://reviewboard.mozilla.org/r/55180/#review52254
Attachment #8756445 - Flags: review?(felipc) → review+
Comment hidden (obsolete)
(Assignee)

Comment 21

a year ago
The above issue is actually more about the change / input events. This one is better: https://github.com/whatwg/html/issues/263

Comment 22

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b53187f3dc44
https://hg.mozilla.org/mozilla-central/rev/b53187f3dc44
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 24

a year ago
Comment on attachment 8756445 [details]
MozReview Request: Bug 1275384 - Dispatch mousedown, mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r?felipe

Approval Request Comment
[Feature/regressing bug #]:

Bug 1266575, which I just requested uplift for.

[User impact if declined]:

If bug 1266575 gets uplifted and this does not, then <select> elements will steal focus after the dropdown closes. For example, this is particularly painful on Bugzilla, where a <select> is used to choose a review flag, and then focus is sent to a text input to put the reviewer email address. Without this patch, focus is stolen from the text input and put back on the <select>.

[Describe test coverage new/current, TreeHerder]:

Manual testing for the focus issue. Automated testing ensures that we're still firing the right events.

[Risks and why]: 

This is very low risk, and only effects users with e10s enabled.

[String/UUID change made/needed]:

None.
Attachment #8756445 - Flags: approval-mozilla-aurora?
status-firefox48: --- → affected
Comment on attachment 8756445 [details]
MozReview Request: Bug 1275384 - Dispatch mousedown, mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r?felipe

Taking it to improve e10s

> only effects users with e10s enabled.
which is going to be a bunch of people very soon ;)
Attachment #8756445 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
has problems to apply to aurora

grafting 347044:b53187f3dc44 "Bug 1275384 - Dispatch mousedown, mouseup and click events manually on <select> for e10s instead of using DOMWindowUtils. r=Felipe"
merging toolkit/modules/SelectContentHelper.jsm
warning: conflicts while merging toolkit/modules/SelectContentHelper.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(mconley)
(Assignee)

Comment 27

a year ago
Yeah, I think the patch for the blocking bug (bug 1266575) needed to be grafted first. This applies cleanly after that.

I'll attempt to re-land this (to Beta, since the uplift just happened) once the trees re-open.
Flags: needinfo?(mconley)
(Assignee)

Comment 28

a year ago
Landed on beta (48):

https://hg.mozilla.org/releases/mozilla-beta/rev/5fe894994e95
status-firefox48: affected → fixed
QA Whiteboard: [good first verify]

Comment 29

11 months ago
I have reproduced on Firefox nightly according to(2016-05-24)

It's fixed and verified on Latest Developer Edition and Beta

Latest Developer Edition--Build ID:( 20160630004007 ), User Agent: 	Mozilla/5.0 (Windows NT 10.0; rv:49.0) Gecko/20100101 Firefox/49.0

Firefox Beta--Build ID:( 20160606200529 ), User Agent: Mozilla/5.0 (Windows NT 10.0; rv:48.0) Gecko/20100101 Firefox/48.0


Tested OS-- Windows10 32bit
QA Whiteboard: [good first verify] → [bugday-20160629]
You need to log in before you can comment on or make changes to this bug.