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

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mconley, Assigned: squib)

Tracking

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

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

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.
(Reporter)

Updated

2 years ago
Depends on: 1321376
(Assignee)

Updated

2 years ago
Blocks: 1349748
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
Ok, the patch here should work. I don't know why Ctrl+Space does bad things to the e10s select widget, but it does.
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
(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.
(Reporter)

Comment 5

2 years ago
mozreview-review
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
Attachment #8850709 - Flags: review?(mconley) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
(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

2 years ago
Pushed by squibblyflabbetydoo@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3192f0f5df95
Update test_bug961363.html to support e10s-based <select> dropdowns; r=mconley

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3192f0f5df95
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.