Closed Bug 1321376 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: Layout: Form Controls, defect)

Product:
Core
Component:
Layout: Form Controls
Version:
50 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(3 files)

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
enndeakin
: review+
Details
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
enndeakin
: review+
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.
58 bytes, text/x-review-board-request
enndeakin
: review+
Details 
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 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 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: nobody → mconley
Status: NEW → ASSIGNED
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 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.
(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 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 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)
Blocks: 1324560
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.
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: