Closed Bug 874819 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(1 obsolete file)

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. :-(
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
Bisected the previously running tests, this is related to running test_docload.html before.
Attached patch Patch v1 (obsolete) — Splinter Review
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?
(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?
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)
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?
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
Closed: 8 years ago
Resolution: --- → WORKSFORME
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 → ---
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.