Update nsListControlFrame in preparation for combining e10s and non-e10s <select> dropdown mechanisms

RESOLVED FIXED in Firefox 53

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

50 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

(Assignee)

Description

7 months ago
Bug 1300784 combines the e10s and non-e10s select dropdown mechanisms. That's good - we're going to have a single code-path there.

However, nsListControlFrame needs to be adjusted somewhat, as do our <select> tests. The following changes are necessary:

1) In order to have the same behaviour when hitting the up and down cursors while the popup is open, I disable changing the <select> value while cursoring if using the parent process popup in single-process mode. This was not necessary in e10s mode because the parent process wasn't dispatching the key events to the content process with the popup open.

2) In single-process mode, UI events are not serialized and sent over IPC, which means we can't rely on the message ordering invariant (messages arrive in order). This means that the Forms:DismissedDropDown message that SelectParentHelper can be sent, and then a key event can be sent to content, but the key event will be handled before the message.

This is as opposed to e10s mode, where the Forms:DismissedDropDown will be received, and then the serialized key event will be handled.

This meant that one of our tests would timeout because the content and parent would be out of sync regarding whether or not the popup was in the open state.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4091681e67f5
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

7 months ago
mozreview-review
Comment on attachment 8815845 [details]
Bug 1321376 - In single-process mode, don't change <select> selection with cursors if popup is opened in the parent.

https://reviewboard.mozilla.org/r/96620/#review97772

::: layout/forms/nsListControlFrame.cpp:2217
(Diff revision 2)
>      case NS_VK_RIGHT:
> +      if (shouldSelectByKey) {
> -      AdjustIndexForDisabledOpt(mEndSelectionIndex, newIndex,
> +        AdjustIndexForDisabledOpt(mEndSelectionIndex, newIndex,
> -                                static_cast<int32_t>(numOptions),
> +                                  static_cast<int32_t>(numOptions),
> -                                1, 1);
> +                                  1, 1);
> +      }

Do we need to handle this for the other keys? 
pagedown/pageup/home/end/etc...

Comment 7

7 months ago
mozreview-review
Comment on attachment 8815846 [details]
Bug 1321376 - Make browser_selectpopup.js wait for SelectContentHelper to think that the popup in the parent has closed when hiding popups.

https://reviewboard.mozilla.org/r/96622/#review97776
Attachment #8815846 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 8

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1996fa888393
Assignee: nobody → mconley
Status: NEW → ASSIGNED
(Assignee)

Comment 9

6 months ago
mozreview-review-reply
Comment on attachment 8815845 [details]
Bug 1321376 - In single-process mode, don't change <select> selection with cursors if popup is opened in the parent.

https://reviewboard.mozilla.org/r/96620/#review97772

> Do we need to handle this for the other keys? 
> pagedown/pageup/home/end/etc...

Good idea - better to be consistent here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

6 months ago
mozreview-review
Comment on attachment 8815845 [details]
Bug 1321376 - In single-process mode, don't change <select> selection with cursors if popup is opened in the parent.

https://reviewboard.mozilla.org/r/96620/#review99844
Attachment #8815845 - Flags: review?(enndeakin) → review+
Why do we need to disable this test? It looks like it only handles cursor navigation is <select> when it is closed.
(Assignee)

Comment 15

6 months ago
(In reply to Neil Deakin from comment #14)
> Why do we need to disable this test? It looks like it only handles cursor
> navigation is <select> when it is closed.

That's what I thought too - but it actually does open it after the first character by hitting the space bar (http://searchfox.org/mozilla-central/rev/a6a339991dbc650bab3aefe939cd0e5ac302274a/layout/forms/test/test_bug961363.html#43).

I think what we might want to do with this test is:

1) Disable the test for now with e10s enabled
2) File a new bug that causes the test to set dom.select_popup_in_parent.enabled to false before running
3) In that same bug, write a new test that tests the desired behaviuor when dom.select_popup_in_parent.enabled is set to true. Have this test run in both e10s and non-e10s mode, and have it set dom.select_popup_in_parent.enabled to true before running.

So what we should end up with is two tests, where one test force-enables and tests the old mechanism, and a second test that tests the new mechanism with its slightly different behaviour.

Sound okay to you?
Flags: needinfo?(enndeakin)

Comment 16

6 months ago
mozreview-review
Comment on attachment 8818665 [details]
Bug 1321376 - Disable test_bug961363.html for e10s mode until we can get a proper test written for the select dropdown in parent behaviour.

https://reviewboard.mozilla.org/r/98648/#review99866

Comment 17

6 months ago
mozreview-review
Comment on attachment 8818665 [details]
Bug 1321376 - Disable test_bug961363.html for e10s mode until we can get a proper test written for the select dropdown in parent behaviour.

https://reviewboard.mozilla.org/r/98648/#review99868
Attachment #8818665 - Flags: review?(enndeakin) → review+
Flags: needinfo?(enndeakin)
(Assignee)

Updated

6 months ago
Blocks: 1324560
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

6 months ago
For posterity sake, the reason that this test wasn't failing before for e10s mode is because the test synthesizes key events for Ctrl-<cursor, home, end, pageup, pagedown> on the content area, and nsListControlFrame.cpp will happily process those events and respond to them, updating the selection. This is, however, not actually possible for the user to do, since the popup for the <select> dropdown should be consuming the events. A more accurate test would have synthesized the keyboard events from the parent side first, to give the popup a chance to consume the events and prevent them from being forwarded to content.

Comment 22

6 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13ef48343bbf
In single-process mode, don't change <select> selection with cursors if popup is opened in the parent. r=enndeakin+6102
https://hg.mozilla.org/integration/autoland/rev/4f21a8556190
Disable test_bug961363.html for e10s mode until we can get a proper test written for the select dropdown in parent behaviour. r=enndeakin+6102
https://hg.mozilla.org/integration/autoland/rev/cf1a77997e44
Make browser_selectpopup.js wait for SelectContentHelper to think that the popup in the parent has closed when hiding popups. r=enndeakin+6102

Comment 23

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/13ef48343bbf
https://hg.mozilla.org/mozilla-central/rev/4f21a8556190
https://hg.mozilla.org/mozilla-central/rev/cf1a77997e44
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.