874819 - (Australis) Failing accessibility focus tests on Windows 7/8 on the UX branch

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

Not on XP though, so possibly aero related:

18:55:07 INFO - 3230 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_focus_general.html | Test timed out.
18:55:15 INFO - 3506 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_focus_menu.xul | uncaught exception - TypeError: can't access dead object at chrome://mochitests/content/a11y/accessible/tests/mochitest/common.js:645
Bug 708927 - Intermittent test_focus_menu.xul | Test timed out, sometimes followed by tens of thousands of gA11yEventListeners is undefined exceptions 18:55:15 INFO - 3516 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_focus_name.html | Unique type focus event was handled.
18:55:15 INFO - 3517 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_focus_name.html | uncaught exception - TypeError: can't access dead object at chrome://mochitests/content/a11y/accessible/tests/mochitest/common.js:645
18:55:16 INFO - 3521 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_focus_selects.html | Unique type focus event was handled.


And a couple of hundred more of those. :-(

Comment 1

[:Gijs (he/him)]

So I suspect the 400-odd failures that follow are because the first test times out and it messes up event queue stuff the other tests rely on. The first failing test (test_focus_general.html)  tries to open the top-level menu by doing a synthetic "alt" keypress. That should show the menubar, and get it a focus event on the first menuitem. On a local build, I can see the menubar being shown, but I don't see a focus event being fired. Not yet sure *why* that doesn't happen, though...

The weirdest is that if I run only test_focus_general.html, everything works fine, also on the UX branch.

Comment 2

[:Gijs (he/him)]

Bisected the previously running tests, this is related to running test_docload.html before.

Comment 3

[:Gijs (he/him)]

Attachment: Patch v1

So reverting these bits of bug 813802 seems to fix this. I've fired off a windows-only try run to verify: https://tbpl.mozilla.org/?tree=Try&rev=f36eb6c9016f . Matt, Mike and Dao, can you please check I did this correctly and/or that it's OK to revert these changes? From reading the bug and a quick check with Mike it seemed like it would be, but I'd rather be safe than sorry. :-)

Comment 4

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

Comment on attachment 752770 [details] [diff] [review]
Patch v1

So, some background here;

Originally, I made those changes because I was under the impression that we wanted the autohidden menubar to maintain its size when the window was in the restored state (having more of a show/hide effect rather than uncollapse/collapse).

I think my original assumption was incorrect. In restored windows, I believe we *do* want the tabs pushed down a bit when showing the menu in the restored state - see bug 870849. If that's the case, then reverting these changes is probably fine.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 752770 [details] [diff] [review]
Patch v1

>--- a/toolkit/content/xul.css
>+++ b/toolkit/content/xul.css
>@@ -272,20 +272,24 @@ toolbar[customizing="true"][hidden="true
> 
> %ifdef XP_MACOSX
> toolbar[type="menubar"] {
>   visibility: collapse;
> }
> %else
> toolbar[type="menubar"][autohide="true"] {
>   -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-menubar-autohide");
>+  overflow:hidden;
> }

nit: Missing space after the colon.

I don't like the xul.css code churn. Can we make it so that bug 813802's current patch doesn't hit mozilla-central in the first place?

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> I don't like the xul.css code churn. Can we make it so that bug 813802's
> current patch doesn't hit mozilla-central in the first place?

AFAIK that'd take a branch reset which would make any other kind of regression really hard to track down, and which I'd rather not do this late in the cycle. Code churn sucks, but I don't think we should go to such lengths to avoid it.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 752770 [details] [diff] [review]
Patch v1

>--- a/toolkit/content/xul.css
>+++ b/toolkit/content/xul.css
>@@ -272,20 +272,24 @@ toolbar[customizing="true"][hidden="true
> 
> %ifdef XP_MACOSX
> toolbar[type="menubar"] {
>   visibility: collapse;
> }

Should this be reverted as well?

Comment 8

[:Gijs (he/him)]

Comment on attachment 752770 [details] [diff] [review]
Patch v1

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 752770 [details] [diff] [review]
> Patch v1
> 
> >--- a/toolkit/content/xul.css
> >+++ b/toolkit/content/xul.css
> >@@ -272,20 +272,24 @@ toolbar[customizing="true"][hidden="true
> > 
> > %ifdef XP_MACOSX
> > toolbar[type="menubar"] {
> >   visibility: collapse;
> > }
> 
> Should this be reverted as well?

I don't think so. OS X isn't really affected by any of this.

Separately, I integrated this with the work for bug 870849 because they're very much related, so I'm obsoleting this and if you think we need to discuss this further, let's do it there.

Comment 9

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #8)
> Comment on attachment 752770 [details] [diff] [review]
> Patch v1
> 
> (In reply to Dão Gottwald [:dao] from comment #7)
> > Comment on attachment 752770 [details] [diff] [review]
> > Patch v1
> > 
> > >--- a/toolkit/content/xul.css
> > >+++ b/toolkit/content/xul.css
> > >@@ -272,20 +272,24 @@ toolbar[customizing="true"][hidden="true
> > > 
> > > %ifdef XP_MACOSX
> > > toolbar[type="menubar"] {
> > >   visibility: collapse;
> > > }
> > 
> > Should this be reverted as well?
> 
> I don't think so. OS X isn't really affected by any of this.

What was the point of changing this for OS X in the first place, if not consistency with the autohiding behavior on other platforms, which you reverted?

Comment 10

[:Gijs (he/him)]

Marking this WFM because for some unknown reason it's been fixed ever since https://hg.mozilla.org/projects/ux/pushloghtml?startID=75&endID=76 . :S

Comment 11

[:Gijs (he/him)]

Annnnd we're back. Gotta love random orange. Hoping for a quick review on 870849 to see if that really does fix this! (I mean, it did, locally...) :-)