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

RESOLVED FIXED

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

Trunk
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Assignee)

Description

5 years ago
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. :-(
(Assignee)

Comment 1

5 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Bisected the previously running tests, this is related to running test_docload.html before.
(Assignee)

Comment 3

5 years ago
Created attachment 752770 [details] [diff] [review]
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. :-)
Attachment #752770 - Flags: review?(mnoorenberghe+bmo)
Attachment #752770 - Flags: review?(dao)
Attachment #752770 - Flags: feedback?(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.
Attachment #752770 - Flags: feedback?(mconley) → feedback+
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?
(Assignee)

Comment 6

5 years ago
(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 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?
(Assignee)

Comment 8

5 years ago
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.
Attachment #752770 - Attachment is obsolete: true
Attachment #752770 - Flags: review?(mnoorenberghe+bmo)
Attachment #752770 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Depends on: 870849
(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?
(Assignee)

Comment 10

5 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 11

5 years ago
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...) :-)
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.