553417 - Write new test for bug 299673

Status: NEW

(Core :: DOM: UI Events & Focus Handling, defect, P5)

Description

[Mats Palmgren (inactive)]

I have disabled the mochitests for bug 299673:
  content/events/test/test_bug299673-1.html
  content/events/test/test_bug299673-2.html
since they are perma/random-orange: bug 553344, bug 524361 and bug 521355.

I tried to fix it using SimpleTest.waitForFocus but it didn't work.
Filing this bug to re-write these tests from scratch.

Comment 1

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)

http://hg.mozilla.org/mozilla-central/rev/22c8fbd5551b

Comment 2

[Martijn Wargers (dead)]

This file:
http://mxr.mozilla.org/mozilla-central/source/dom/events/test/bug299673.js
has to be rewritten to use pushPrefEnv, but since it's disabled.

http://mxr.mozilla.org/mozilla-central/source/dom/events/test/mochitest.ini#24
http://mxr.mozilla.org/mozilla-central/source/dom/events/test/test_bug299673-1.html?force=1
http://mxr.mozilla.org/mozilla-central/source/dom/events/test/test_bug299673-2.html?force=1

Comment 3

[Martijn Wargers (dead)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=433fa28473f3

Comment 4

[Martijn Wargers (dead)]

Attachment: 553417.diff

This makes the test pass locally for me on MacOSx10.10 debug.

Comment 5

[Martijn Wargers (dead)]

Comment on attachment 8724778 [details] [diff] [review]
553417.diff

Tryservers shows that the test is failing on linux, e10s and Windows. It's passing on Mac.
Mats, is it ok to re-enable these tests like this for Mac, or did you mean something else with "re-write these tests from scratch"?

Comment 6

[Mats Palmgren (inactive)]

I think I meant that someone needs to investigate what the results for this
test is supposed to be according to relevant specs (if any) and compare
what other UAs are doing.  It certainly seems wrong (to me) that we get
different results on different platforms.  It may be that the test is poorly
written, or it may be an actual underlying bug here that we need to fix.

What the test is doing is this: in the top document we have a <select size=1>
and (after it) an <input type=text>.  First we focus the <select>, then we
synthesize a VK_DOWN (to change the selected value) and then a VK_TAB.
The VK_TAB should cause a 'change' event followed by a 'blur' on the <select>
and then 'focus' on the <input>.  However, there is an onchange handler on
the <select> that opens a popup window, which will blur the top document
and its currently focused element, and focus its document instead.
The question is, in what order should all these focus/blur events occur?

Here the test results from (non-E10S) Linux32 (that failed), FTR:
(edited for readability)
===============================
FAIL | dom/events/test/test_bug299673-1.html | wrong element is focused after popup was closed - got [object HTMLInputElement], expected [object HTMLBodyElement]


FAIL | dom/events/test/test_bug299673-1.html | expected events - got " :  
Test with browser.link.open_newwindow = 2 
focus top-doc
SELECT(Select1): focus
SELECT(Select1): change   
>>> OpenWindow  
<<< OpenWindow
SELECT(Select1):  blur
INPUT(Text1): focus
INPUT(Text1): blur  
blur top-doc 
focus popup-doc
INPUT(popupText1): focus
INPUT(popupText1): blur  
blur popup-doc 
focus top-doc
INPUT(Text1): focus", 

expected " :  
Test with browser.link.open_newwindow = 2 
focus top-doc
SELECT(Select1): focus
SELECT(Select1): change   
>>> OpenWindow 
blur top-doc 
focus popup-doc
INPUT(popupText1): focus   
<<< OpenWindow
SELECT(Select1): blur
INPUT(popupText1): blur  
blur popup-doc 
focus top-doc"
===============================

The "OpenWindow" message is from the function that does the window.open() call:
http://mxr.mozilla.org/mozilla-central/source/dom/events/test/bug299673.js#3

If you look at the actual result, it seems that the focus() calls inside the popup
window are now asynchronous and that we return before reaching "focus popup-doc"
etc. and that we process the TAB in the top document (SELECT(Select1):  blur,
INPUT(Text1): focus) before the focus stuff in the popup window occurs.

The expected results seems to be expecting a more synchronous behavior, i.e.
that the "SELECT(Select1): blur" happens after OpenWindow returns (which runs
inside onchange).

I don't know which behavior is "correct" here.

(I guess it should be possible to run this test manually in other UAs
by focusing the <select>, type the DOWN arrow key then TAB.  It might
need some tweaks though.)

Also note that other UAs might fire the 'change' event already on the DOWN key.
It should be possible to compare the order of events between the top/popup
documents anyway I hope.

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8724778 [details] [diff] [review]
553417.diff

The pushPrefEnv fix in bug299673.js looks good to me. (r=mats on that part)

I don't think we should enable these tests until we know what
the problem is and what other UAs do. (r- on those parts for now)

Comment 8

[Martijn Wargers (dead)]

Attachment: 553417.diff

Ok, this is without enabling the tests.

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8726756 [details] [diff] [review]
553417.diff

But the test is already enabled in the manifest, although at the moment
it just runs that todo().  These changes *will* run the test AFAICT.
Am I missing something?

Comment 10

[Martijn Wargers (dead)]

Attachment: 553417.diff

You're right, sorry about that. I forgot about it, that this was disabled on the test level instead of the manifest level.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8726786 [details] [diff] [review]
553417.diff

While we're here -- please add a ", see bug 553417" at the end of
the todo() and leave this bug open after landing since it contains
useful info on what remains to be done.  Thanks.

Also, you probably want to modify the commit message so that it only
says "rewrite tests to use pushPrefEnv".

Comment 12

[Martijn Wargers (dead)]

Attachment: 553417.diff (checked in)

With comment 11 addressed.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc5bb93e4e9

Comment 14

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/9cc5bb93e4e9

Comment 15

[Martijn Wargers (dead)]

Attachment: test_bug299673-1.html

On Google Chrome and Safari, pressing the down key opens the select popup, and then the tab key doesn't do anything. You have to press the Return, space or esc key to get the select popup to be dismissed.

Comment 16

[Martijn Wargers (dead)]

Microsoft Edge (and I think Internet Explorer as well) opens the popup window as soon as press the down key, the result after that is this:
[object HTMLSelectElement]:(Select1): focus
[object HTMLSelectElement]:(Select1): change
>>> OpenWindow
[object HTMLSelectElement]:(Select1): blur
[object HTMLInputElement]:(popupText1): focus
[object HTMLSelectElement]:(Select1): change top-doc
<<< ClosePopup
[object HTMLInputElement]:(popupText1): change popup-doc
[object HTMLSelectElement]:(Select1): focus
[object HTMLInputElement]:(popupText1): blur 

Regarding the results in comment 15, they are for Firefox trunk on Mac.

I don't know about if there is any spec that is telling how UA's should behave here. I don't think there is.
I currently am not able to test on Linux.

Let me know if you need more info and what I need to do to enable these tests.

Comment 17

[Mats Palmgren (inactive)]

The HTML spec describes some events for <select>, I haven't looked in detail what it says though.
And in our case there is also window focus management involved which I'm not sure how much
is specified, if at all (the HTML spec is probably where it would be if it exists).
https://html.spec.whatwg.org/multipage/forms.html#the-select-element

> On Google Chrome and Safari, pressing the down key opens the select popup [...]

Interesting.  That's on OSX I presume.  Chrome on Linux does NOT do that -- it changes
the value to the next one in the list without opening the dropdown menu:

Current output:
[object HTMLSelectElement]:(Select1): focus 
[object HTMLSelectElement]:(Select1): change 
>>> OpenWindow
[object HTMLSelectElement]:(Select1): change top-doc
[object HTMLSelectElement]:(Select1): blur 
[object HTMLInputElement]:(popupText1): focus 
[object HTMLInputElement]:(popupText1): blur 
<<< ClosePopup
[object HTMLSelectElement]:(Select1): focus 

> Microsoft Edge (and I think Internet Explorer as well) opens the popup window as soon as press
> the down key

OK, that's what we should do too on all platforms, except maybe OSX if KEY_DOWN generally
opens the menu instead (in native UI etc) on that platform.  Is there another key-combo
to change the value directly (without opening the menu) on OSX?

I think that's bug 126379.  Unfortunately I won't have time to work on that for
the foreseeable future.

I'm not sure there is much value in these tests before that happens, but feel free
to make it work and enable it on some platforms if you have the time.  At least it
protects us from regressing the behavior that we currently have, even it that
behavior is currently wrong.  rs=mats on anything that's green on Try.

Comment 18

[Martijn Wargers (dead)]

(In reply to Mats Palmgren (:mats) from comment #17)
> OK, that's what we should do too on all platforms, except maybe OSX if
> KEY_DOWN generally
> opens the menu instead (in native UI etc) on that platform.  Is there
> another key-combo
> to change the value directly (without opening the menu) on OSX?

pressing the 'o' key opens the new window popup and changes the select value. The output I get on Chrome is:
[object HTMLSelectElement]:(Select1): focus 
[object HTMLSelectElement]:(Select1): change 
>>> OpenWindow
[object HTMLSelectElement]:(Select1): change top-doc
[object HTMLSelectElement]:(Select1): blur 
[object HTMLInputElement]:(popupText1): focus 
<<< ClosePopup
[object HTMLSelectElement]:(Select1): focus 
[object HTMLSelectElement]:(Select1): blur

Comment 19

[Mats Palmgren (inactive)]

OK, so it does have that behavior for "changing the value using a kbd command" then,
only it's not KEY_DOWN specifically.  That's good to know, thanks.  I'm even more
convinced we need to fix bug 126379 then.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:hsinyi, maybe it's time to close this bug?

Comment 21

[Hsin-Yi Tsai (she/her)[:hsinyi]]

There's still a open dependency bug. It doesn't sound right to close this.
:mats, please feel free to chime in.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:hsinyi, maybe it's time to close this bug?

Comment 23

[Hsin-Yi Tsai (she/her)[:hsinyi]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #22)

The leave-open keyword is there and there is no activity for 6 months.
:hsinyi, maybe it's time to close this bug?

no.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:hsinyi, maybe it's time to close this bug?

Comment 25

[Hsin-Yi Tsai (she/her)[:hsinyi]]

no

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jstutte, maybe it's time to close this bug?

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jstutte, maybe it's time to close this bug?

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:hsinyi, maybe it's time to close this bug?

Comment 29

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:hsinyi, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 30

[Hsin-Yi Tsai (she/her)[:hsinyi]]

(In reply to Release mgmt bot [:marco/ :calixte] from comment #29)

The leave-open keyword is there and there is no activity for 6 months.
:hsinyi, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

No, let's keep this open.

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.