1275384 - [e10s] bugzilla text boxes fail to take focus

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Jim Mathies [:jimm]]

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?

Comment 1

[blinky]

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

Regressed by: bug 1266575

Comment 2

[Neil Deakin]

If its a regression, let's ask Mike then!

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Investigating.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Manual test case

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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/

Comment 6

[:Felipe Gomes (needinfo for replies!)]

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

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(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?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Hmm, totally bogus coordinates sounds odd. But ok fine if others do that do.

What about event.buttons?

Comment 10

[Jim Mathies [:jimm]]

Tracy, please track down a regression range on this.

Comment 11

[Jim Mathies [:jimm]]

(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

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Yeah, I'll look at those properties.

One thing I've realized, however, is that for Blink, mousedown and mouseup is never fired after an <option> is selected - only click. I think the only reason we were sending mousedown/mouseup was to "naturally" generate the click event.

I'll remove mousedown and mouseup to bring us more in line with what Blink is doing.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

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/

Comment 15

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

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/

Comment 19

[:Felipe Gomes (needinfo for replies!)]

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

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Hopefully when https://github.com/whatwg/html/issues/1092 resolves, there'll be clarity on what's expected from user agents.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

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

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b53187f3dc44

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b53187f3dc44

Comment 24

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

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 ;)

Comment 26

[Carsten Book [:Tomcat]]

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')

Comment 27

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Landed on beta (48):

https://hg.mozilla.org/releases/mozilla-beta/rev/5fe894994e95

Comment 29

[Saheda Reza [:Antora]]

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