1324560 - Split test_bug961363.html and create old dropdown and new dropdown variants

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

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

Bug 1321376 and bug 1300784 pave the way for us to combine both the e10s and non-e10s <select> dropdown mechanisms.

The behaviour of the new dropdown is slightly different to the old one wrt keyboard interaction. test_bug961363.html tests (among other things), the old behaviour. We disabled test_bug961363.html for e10s in bug 1321376 so we could get bug 1321376 and bug 1300785 landed, but we should probably revive this test for the new world (certainly before we enable the new popup in non-e10s mode by default).

Here's how I propose we go about doing this:

1) Split out the <select multiple> tests into their own test - this can run the same way in e10s and non-e10s mode.
2) Have test_bug961363.html not run with e10s enabled, and also have it set the dom.select_popup_in_parent.enabled pref to false by default. This will allow us to continue testing the old behaviour
3) Write a new test that exercises the new behaviour, and make sure it passes with dom.select_popup_in_parent.enabled set to True for e10s and non-e10s.

So this is the bug for doing the above.

Comment 1

[Jim Porter (:squib)]

Attachment: Bug 1324560 - Update test_bug961363.html to support e10s-based <select> dropdowns;

The split tests (test_select_key_navigation_(non)?_e10s_bug961363.html differ
only in that the e10s version doesn't synthesize ctrl+space after testing
ctrl+(direction key). Doing so was the cause of the test failures prior to
this commit.

Review commit: https://reviewboard.mozilla.org/r/123248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123248/

Comment 2

[Jim Porter (:squib)]

Ok, the patch here should work. I don't know why Ctrl+Space does bad things to the e10s select widget, but it does.

Comment 3

[Jim Porter (:squib)]

Comment on attachment 8850709 [details]
Bug 1324560 - Update test_bug961363.html to support e10s-based <select> dropdowns;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123248/diff/1-2/

Comment 4

[Jim Porter (:squib)]

(In reply to Jim Porter (:squib) from comment #2)
> Ok, the patch here should work. I don't know why Ctrl+Space does bad things
> to the e10s select widget, but it does.

I figured it out. Ctrl+Space is processed as just Space, which opens the dropdown and triggers all the (intentional) behavioral changes that come with the new e10s-backed dropdown. I've updated the tests in light of this and they should be good now.

Comment 5

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

Comment on attachment 8850709 [details]
Bug 1324560 - Update test_bug961363.html to support e10s-based <select> dropdowns;

https://reviewboard.mozilla.org/r/123248/#review127436

Thanks for the patch! Way more readible to boot. Good stuff!

::: layout/forms/test/test_select_key_navigation_bug961363.html:29
(Diff revision 2)
> -                  {k:"RIGHT",s:[false,false,true,false]}, {k:"LEFT",s:[false,true,false,false]},
> -                  {k:"END",s:[false,false,false,true]}, {k:"HOME",s:[true,false,false,false]} ];
> -     var two_2 = [{k:"PAGE_DOWN",s:[true,false,false,false]}, {k:"PAGE_UP",s:[true,false,false,false]}];
> -     var three_1 = [{k:"DOWN",s:[false,false,true,false]}, {k:"UP",s:[false,true,false,false]},
> -                    {k:"RIGHT",s:[false,false,true,false]}, {k:"LEFT",s:[false,true,false,false]},
> -                    {k:"END",s:[false,false,false,true]}, {k:"HOME",s:[true,false,false,false]} ];
> +        {key: "END",       change: true,  state: [false, false, false, true ]},
> +        {key: "HOME",      change: true,  state: [true,  false, false, false]},
> +        {key: "PAGE_DOWN", change: false, state: [true,  false, false, false]},
> +        {key: "PAGE_UP",   change: false, state: [true,  false, false, false]}
> +      ];
> +      const single_dropdown = [

Nit: Let's put another newline above this to be consistent with the `const multiple` down below.

::: layout/forms/test/test_select_key_navigation_bug961363.html:79
(Diff revision 2)
> +          is(selected.toString(), data.state.toString(),
> +             `selected options match after ${action} (id: ${id})`);

Alternatively, I believe you could use Assert.equal to compare these two arrays instead of converting to strings, but meh

Comment 6

[Jim Porter (:squib)]

Comment on attachment 8850709 [details]
Bug 1324560 - Update test_bug961363.html to support e10s-based <select> dropdowns;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123248/diff/2-3/

Comment 7

[Jim Porter (:squib)]

(In reply to Mike Conley (:mconley) (Very high latency until April 3rd) from comment #5)
> Nit: Let's put another newline above this to be consistent with the `const
> multiple` down below.

Fixed.

> ::: layout/forms/test/test_select_key_navigation_bug961363.html:79
> (Diff revision 2)
> > +          is(selected.toString(), data.state.toString(),
> > +             `selected options match after ${action} (id: ${id})`);
> 
> Alternatively, I believe you could use Assert.equal to compare these two
> arrays instead of converting to strings, but meh

I ended up leaving this as-is since it looks like Assert.equal isn't available in mochitests (or else I need to do something special to get it).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3192f0f5df95
Update test_bug961363.html to support e10s-based <select> dropdowns; r=mconley

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3192f0f5df95