Open Bug 553417 Opened 14 years ago Updated 20 days ago

Write new test for bug 299673

Categories

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

defect

Tracking

()

People

(Reporter: MatsPalmgren_bugz, Unassigned)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Attached patch 553417.diff (obsolete) — Splinter Review
This makes the test pass locally for me on MacOSx10.10 debug.
Assignee: nobody → martijn.martijn
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"?
Attachment #8724778 - Flags: feedback?(mats)
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 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)
Attachment #8724778 - Flags: feedback?(mats) → feedback+
Attached patch 553417.diff (obsolete) — Splinter Review
Ok, this is without enabling the tests.
Attachment #8724778 - Attachment is obsolete: true
Attachment #8726756 - Flags: review?(mats)
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?
Flags: needinfo?(martijn.martijn)
Attached patch 553417.diff (obsolete) — Splinter Review
You're right, sorry about that. I forgot about it, that this was disabled on the test level instead of the manifest level.
Attachment #8726756 - Attachment is obsolete: true
Attachment #8726756 - Flags: review?(mats)
Flags: needinfo?(martijn.martijn)
Attachment #8726786 - Flags: review?(mats)
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".
Attachment #8726786 - Flags: review?(mats) → review+
With comment 11 addressed.
Attachment #8726786 - Attachment is obsolete: true
Attachment #8726811 - Attachment description: 553417.diff (for check-in) → 553417.diff (checked in)
Attached file 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.
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.
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats)
(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
Depends on: 126379
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.
The leave-open keyword is there and there is no activity for 6 months.
:hsinyi, maybe it's time to close this bug?
Flags: needinfo?(htsai)
There's still a open dependency bug. It doesn't sound right to close this.
:mats, please feel free to chime in.
Flags: needinfo?(htsai)
Component: Event Handling → User events and focus handling

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

Flags: needinfo?(htsai)

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

Flags: needinfo?(htsai)

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

Flags: needinfo?(htsai)

no

Flags: needinfo?(htsai)

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

Flags: needinfo?(jstutte)
Flags: needinfo?(jstutte)

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

Flags: needinfo?(jstutte)
Flags: needinfo?(jstutte)

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

Flags: needinfo?(htsai)
Flags: needinfo?(htsai)

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.

Flags: needinfo?(htsai)

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

Flags: needinfo?(htsai)
Keywords: leave-open
Priority: -- → P5
Severity: normal → S3

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

Assignee: martijn.martijn → nobody
Attachment #9387385 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.