1180761 - [e10s] Space to mark checkbox scrolls page too

Status: RESOLVED FIXED

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

Description

[Elbart]

STR:
- Visit URL
- Click "Log in or Sign up"
- Hit tab twice, the checkbox for "Stay logged in" should be focused
- Hit space

ER:
The checkbox is marked.

AR:
The checkbox is marked and the page is scrolled down.

Regression-range:

m-c:
Last good revision: 946ac85af8f4 (2015-04-22)
First bad revision: 0b202671c9e2 (2015-04-23)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=946ac85af8f4&tochange=0b202671c9e2

m-i:
Last good revision: 13341fae85ed
First bad revision: 984018eae04a
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=13341fae85ed&tochange=984018eae04a

79aeff6684f6	Ehsan Akhgari — Bug 915962 - Part 1: Allow pressing space to scroll the document if an editable element or form control is not focused; r=roc

Comment 1

[(no longer active)]

Hmm, Neil, do you know how event handling is different on e10s?  Specifically, it seems like returning here <http://hg.mozilla.org/mozilla-central/rev/79aeff6684f6#l1.62> is not enough to tell the browser to not scroll.  What else should be done here?

Comment 2

[Elbart]

Attachment: Testcase

Testcase.
Click anywhere, hit tab to focus the checkbox, and space to mark it.

Comment 3

[Neil Deakin]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #1)
> Hmm, Neil, do you know how event handling is different on e10s? 
> Specifically, it seems like returning here
> <http://hg.mozilla.org/mozilla-central/rev/79aeff6684f6#l1.62> is not enough
> to tell the browser to not scroll.  What else should be done here?

Looks like this code is being run in the child process correctly and then again in the parent process, where the focused element is a <browser>, so the form control check fails.

Comment 4

[(no longer active)]

I'm not the right person to fix this.  I am not sure if I even understand comment 3, and have no clue what it would take to fix this.

Comment 8

[Neil Deakin]

Attachment: Move special case into controller

Don't really like having to add the special commands to handle the 'browser.backspace_action' but can't think of a better way right now.

Comment 9

[[e-mail]]

(In reply to Neil Deakin from comment #8)
> Move special case into controller
So how does this change actually end up fixing the bug?

> Don't really like having to add the special commands to handle the
> 'browser.backspace_action' but can't think of a better way right now.
Surely this breaks the Page Up and Page Down keys when a form control is focused? Maybe add special commands to handle the space action instead? (Why) Doesn't the form control prevent the space action anyway?

Also what's with the change to IsCommandEnabled?

Comment 10

[Neil Deakin]

Attachment: Different approach

This is a simpler fix.

Comment 11

[[e-mail]]

Comment on attachment 8667029 [details] [diff] [review]
Different approach

Wow, that was simple!

Comment 12

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fff7b56c8659f0ea90fefdb772095f633295723
Bug 1180761, cancel the event earlier so that space doesn't trigger checkbox change and scroll, r=neil

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fff7b56c865

Comment 14

[Fahmida Noor]

I have reproduced this bug with Firefox Nightly 42.0a1 (Build ID : 20150706030206) on Windows 7(64-bit) with the instructions from comment 0.

Verified as fixed with Latest Firefox Beta 44.0b9 (Build ID : 20160114165817)

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0

[testday-20160115]