Last Comment Bug 1321376 - Update nsListControlFrame in preparation for combining e10s and non-e10s <select> dropdown mechanisms
: Update nsListControlFrame in preparation for combining e10s and non-e10s <sel...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: 50 Branch
: Unspecified Unspecified
-- normal (vote)
: mozilla53
Assigned To: Mike Conley (:mconley)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 1324560 1300784
  Show dependency treegraph
 
Reported: 2016-11-30 11:51 PST by Mike Conley (:mconley)
Modified: 2016-12-20 11:37 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Bug 1321376 - In single-process mode, don't change <select> selection with cursors if popup is opened in the parent. (58 bytes, text/x-review-board-request)
2016-11-30 11:54 PST, Mike Conley (:mconley)
enndeakin: review+
Details | Review
Bug 1321376 - Make browser_selectpopup.js wait for SelectContentHelper to think that the popup in the parent has closed when hiding popups. (58 bytes, text/x-review-board-request)
2016-11-30 11:54 PST, Mike Conley (:mconley)
enndeakin: review+
Details | Review
Bug 1321376 - Disable test_bug961363.html for e10s mode until we can get a proper test written for the select dropdown in parent behaviour. (58 bytes, text/x-review-board-request)
2016-12-14 13:37 PST, Mike Conley (:mconley)
enndeakin: review+
Details | Review

Description User image Mike Conley (:mconley) 2016-11-30 11:51:07 PST
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 1 User image Mike Conley (:mconley) 2016-11-30 11:54:17 PST Comment hidden (mozreview-request)
Comment 2 User image Mike Conley (:mconley) 2016-11-30 11:54:17 PST Comment hidden (mozreview-request)
Comment 4 User image Mike Conley (:mconley) 2016-11-30 14:37:47 PST Comment hidden (mozreview-request)
Comment 5 User image Mike Conley (:mconley) 2016-11-30 14:37:47 PST Comment hidden (mozreview-request)
Comment 6 User image Neil Deakin 2016-12-06 15:12:05 PST
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 User image Neil Deakin 2016-12-06 15:18:03 PST
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
Comment 9 User image Mike Conley (:mconley) 2016-12-14 13:37:44 PST
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 10 User image Mike Conley (:mconley) 2016-12-14 13:37:59 PST Comment hidden (mozreview-request)
Comment 11 User image Mike Conley (:mconley) 2016-12-14 13:37:59 PST Comment hidden (mozreview-request)
Comment 12 User image Mike Conley (:mconley) 2016-12-14 13:37:59 PST Comment hidden (mozreview-request)
Comment 13 User image Neil Deakin 2016-12-19 07:00:25 PST
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
Comment 14 User image Neil Deakin 2016-12-19 07:08:28 PST
Why do we need to disable this test? It looks like it only handles cursor navigation is <select> when it is closed.
Comment 15 User image Mike Conley (:mconley) 2016-12-19 08:35:04 PST
(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?
Comment 16 User image Neil Deakin 2016-12-19 08:55:09 PST
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 User image Neil Deakin 2016-12-19 08:55:16 PST
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
Comment 18 User image Mike Conley (:mconley) 2016-12-19 13:18:24 PST Comment hidden (mozreview-request)
Comment 19 User image Mike Conley (:mconley) 2016-12-19 13:18:24 PST Comment hidden (mozreview-request)
Comment 20 User image Mike Conley (:mconley) 2016-12-19 13:18:24 PST Comment hidden (mozreview-request)
Comment 21 User image Mike Conley (:mconley) 2016-12-19 13:21:10 PST
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 User image Pulsebot 2016-12-19 13:21:20 PST
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

Note You need to log in before you can comment on or make changes to this bug.