1415787 - Fix failure in dom/svg/test/test_tabindex.html on OSX with comformant Promise handling

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

This is a follow-up of the try result in bug 1193394 comment 58:
(M3 in osx build: https://treeherder.mozilla.org/logviewer.html#?job_id=143221153&repo=try&lineNumber=8790)

TEST-UNEXPECTED-FAIL | dom/svg/test/test_tabindex.html | The active element tabindex is 3 - got -1, expected 3

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

-1 on Mac seems to be expected result according to the comment in:
https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/svg/test/test_tabindex.html#86-88

Comment 2

[Hiroyuki Ikezoe (:hiro)]

The comment is;

      // This test has to be run with other tests, otherwise,
      // document.activeElement.tabIndex will be -1 on Mac.
      is(document.activeElement.tabIndex, 3, "The active element tabindex is 3");

Daosheng, what the comment supposed to mean?
It seems to me that the test is originally flaky (there is an intermittent bug, buAndg 1377521).  I think we should wait for focus before starting the test, and I confirmed locally that the test fails if the test starts after waiting for the focus.  (I did run the test linux with pretending Mac)

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch: Wait for the focus before starting the test

Yes, I can confirm that it's always '-1' if we start the test after focus with this patch on Mac.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Link to the bug that introduced the failure case.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I think there needs more focusable elements for Mac.  I think if there is no more focusable elements when tab key pressed, focus goes away from the document, right?

Here is a try with more focusable elements.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2785ab9952f87761cd9fa724c6c418d6126f926

Comment 6

[Hiroyuki Ikezoe (:hiro)]

And another try with the conformant Promise handling;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4617f05f8314a25b69d2f6b51d8e3b7e6d76f94f

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Both tries look pretty good.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415787 - Add two extra elements in the SVG element to avoid losing focus.

On MacOSX, more precisely if full keyboard access is not enabled on MacOSX,
we skip focusing link elements in SVG with tab key, so if there is no more
elements to be focused in the SVG, the focus will be moved outside the SVG.
To prevent this situation, there needs more focusable elements in the SVG.

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1415787 - Wait for focus before proceeding test.

To process focus handling deterministic, we need to wait for focus before
proceeding test.

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

Comment 10

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8938217 [details]
Bug 1415787 - Add two extra elements in the SVG element to avoid losing focus.

https://reviewboard.mozilla.org/r/208974/#review214746

LGTM. Thanks!

Comment 11

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8938218 [details]
Bug 1415787 - Wait for focus before proceeding test.

https://reviewboard.mozilla.org/r/208976/#review214748

thanks!

Comment 12

[Daosheng Mu[:daoshengmu]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> The comment is;
> 
>       // This test has to be run with other tests, otherwise,
>       // document.activeElement.tabIndex will be -1 on Mac.
>       is(document.activeElement.tabIndex, 3, "The active element tabindex is
> 3");
> 
> Daosheng, what the comment supposed to mean?
> It seems to me that the test is originally flaky (there is an intermittent
> bug, buAndg 1377521).  I think we should wait for focus before starting the
> test, and I confirmed locally that the test fails if the test starts after
> waiting for the focus.  (I did run the test linux with pretending Mac)

You got the main point. Thanks for help!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ca3a3798417
Add two extra elements in the SVG element to avoid losing focus. r=daoshengmu
https://hg.mozilla.org/integration/autoland/rev/27b617e2cad7
Wait for focus before proceeding test. r=daoshengmu

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ca3a3798417
https://hg.mozilla.org/mozilla-central/rev/27b617e2cad7