Last Comment Bug 781090 - Firefox no longer responds to Spectacle's keyboard shortcuts to resize application windows
: Firefox no longer responds to Spectacle's keyboard shortcuts to resize applic...
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Disability Access (show other bugs)
: 16 Branch
: x86 Mac OS X
: -- normal with 1 vote (vote)
: Firefox 17
Assigned To: Hubert Figuiere [:hub]
:
Mentors:
http://spectacleapp.com/
: 782974 (view as bug list)
Depends on: 761589
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-07 22:16 PDT by Chris Peterson [:cpeterson]
Modified: 2012-08-23 06:53 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
fixed


Attachments
Have minimum a11y support when we don't have Voice Over. r= (3.73 KB, patch)
2012-08-17 18:10 PDT, Hubert Figuiere [:hub]
dbolter: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2012-08-07 22:16:34 PDT
"Spectacle" is a Mac utility that provides keyboard shortcuts for resizing application windows.

STR:
1. Install Spectacle from spectacleapp.com or Apple's Mac App Store
2. Launch Firefox
3. Try to use Spectacle's Cmd + Alt + ← and Cmd + Alt + → keyboard shortcuts to resize Firefox's application window.

AR:
Spectacle works correctly in Firefox 14 and Beta 15, but not Aurora 16 or Nightly 17. I am running OS X 10.8.0 (Mountain Lion), but I believe I have also seen the problem on OS X 10.7.4 (Lion)

I bisected Nightly builds from ftp.mozilla.org and I believe this bug was a regression in Nightly 16's 2012-06-20 build. Here is the pushlog from Nightly 06-19 to 06-20:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=373e6f9264e6&tochange=c3190d715044
Comment 1 Chris Peterson [:cpeterson] 2012-08-07 22:24:54 PDT
Hub, I see that your bug 761589 made changes to accessibility.force_disabled in the regression build 06-20. Do you think your change may have caused this bug or just revealed a preexisting problem?

If I set accessibility.force_disabled=-1, then Spectacle's keyboard shortcuts start working again.
Comment 2 Marco Zehe (:MarcoZ) on PTO until August 15 2012-08-08 00:35:22 PDT
I looked in the regression pushlog and didn't find Mac-related accessibility fixes. I did find, however, bug 757618, which directly deals with something app window related. CC'ing Steven.
Comment 3 Marco Zehe (:MarcoZ) on PTO until August 15 2012-08-08 00:41:16 PDT
Oops, sorry, overlooked that there was comment #1. Please ignore last comment.

Yes, we did make a change in 16 that only invokes accessibility when VoiceOver is used, but doesn't turn it on otherwise, *unless* the value is set to -1 for acc essibility-force_disabled. This has to do with severe perf regressions introduced by turning on accessibility by default in Mac builds.
Comment 4 Hubert Figuiere [:hub] 2012-08-08 10:22:33 PDT
Firefox 14 and 15 do NOT have Accessibility built in on Mac. Firefox 16 has it, but it only really gets enabled when VoiceOver is running.

Setting accessibility.force_disabled=-1 actually bypass that whitelisting.

As Marco said, the whitelisting is necessary as it seems there are tools like input methods that seems to be causing large performance issues when Accessibility is enabled.

Now why does it work with a11y not even compiled in, but fail with it in is a mystery.

Has anyone contacted Spectacle author?
Comment 5 David Bolter [:davidb] 2012-08-08 11:08:46 PDT
Spectacle might be using feature detection, perhaps choosing a11y API first if it works. Aside: I wonder if there is a way for tools like this to spoof VO running, since that could be an existing trick to use on Chrome for mac.
Comment 6 Chris Peterson [:cpeterson] 2012-08-08 11:34:35 PDT
(In reply to David Bolter [:davidb] (I'm back!) from comment #5)
> Spectacle might be using feature detection, perhaps choosing a11y API first
> if it works. Aside: I wonder if there is a way for tools like this to spoof
> VO running, since that could be an existing trick to use on Chrome for mac.

Chrome works correctly with Spectacle.

Spectacle is open source. The code is available on github:

  https://github.com/eczarny/spectacle

AFAICT, the window resizing magic happens in Spectacle's `moveFrontMostWindowWithAction` method:

  https://github.com/eczarny/spectacle/blob/master/SpectacleWindowPositionManager.m#L105
Comment 7 Hubert Figuiere [:hub] 2012-08-08 11:36:14 PDT
All they need is to set AXEnhancedUserInterface to 1 on the AXApplication object. But this will cause all the know issue in a11y on Mac.
Comment 8 Hubert Figuiere [:hub] 2012-08-16 08:40:26 PDT
*** Bug 782974 has been marked as a duplicate of this bug. ***
Comment 9 Andreas H. 2012-08-16 09:34:54 PDT
Unfortunately even with accessibility.force_disabled=-1  it doesn't work quite like it did in Firefox 14. Getting accessibility information doesn't work for the "website" part of the window, only while hovering over the top part (tab bar, bookmark bar etc.). (You can test this with apples accessibility inspector app)

Most apps don't even need all the information that can be returned by the Accessibility API, the only important object is the AXWindow with the settable attributes AXSize and AXPosition and if possible the AXTitle. So I have absolutely no idea how this stuff works, but maybe it would be possible to just always return the AXWindow item. (like in this Screenshot: http://d.pr/i/3ngQ/5vv3LwmL )
Comment 10 Andreas H. 2012-08-17 08:50:13 PDT
Ah, I had an extension installed which for some reason prevented this from working (TileTabs https://addons.mozilla.org/de/firefox/addon/tile-tabs/)

After uninstalling, the accessibility features now work as expected while using accessibility.force_disabled=-1 or when setting AXEnhancedUserInterface to 1.

So my app will probably automatically change the AXEnahncedUserInterface in order to keep working with Firefox... personally I've never experienced big performance issues with Firefox while using this. But maybe I'll popup a warning to my users...
Comment 11 Hubert Figuiere [:hub] 2012-08-17 09:49:08 PDT
> So my app will probably automatically change the AXEnahncedUserInterface in
> order to keep working with Firefox... 

Please DO NOT DO THAT.
Comment 12 Andreas H. 2012-08-17 11:46:21 PDT
but what are my options if I don't want x users to complain about it not working with Firefox? It has been working for all Firefox versions I know of until now, so the users won't understand why it's not working anymore.
Comment 13 Hubert Figuiere [:hub] 2012-08-17 12:11:52 PDT
Beat me to it and propose a patch to Firefox to fix the problem.

Because if you do the above, users will not understand why Firefox suddenly has problems. It will do a LARGE disservice to Firefox users as well, and I can tell you that I could be tempted to use a different white-listing (very simple fix) to make sure this doesn't happen.
Comment 14 Andreas H. 2012-08-17 13:30:43 PDT
If I'd set AXEnhancedUserInterface to 1 everything would be like in Firefox 14 (which worked perfectly for me), or did I get that wrong?

There are many window resizing/moving tools out there and I think keeping the accessibility features turned off by default may result in quite a few complaining users when this goes live. (Especially because it has been working for such a long time now)

Is there an easy way to reproduce the performance issues on Mac OS X? I'd love to help with a patch but currently I have no idea about how Firefox and maca11y work internally so I think I'd have to invest much time to get a basic understanding... But I'll try.

For now I'll probably add an intermediate workaround to my tools, which will use the kAXFocusedApplicationAttribute and the kAXFocusedWindowAttribute in case AXUIElementCopyElementAtPosition fails. This still works with accessibility disabled in Firefox, and hopefully most of the window resizing tools out there already use this to get the AXWindow they need. 
Unfortunately this won't give me the window under the cursor which is necessary for some actions. (There is another way to get the window under the cursor but I want to avoid that for performance reasons...)

While hovering over the Firefox titlebar I can still get the Firefox AXWindow via AXUIElementCopyElementAtPosition() while accessibility is disabled in Firefox, but that's probably because this is standard Cocoa stuff, isn't it?
Comment 15 Hubert Figuiere [:hub] 2012-08-17 14:24:18 PDT
(In reply to Andreas H. from comment #14)
> If I'd set AXEnhancedUserInterface to 1 everything would be like in Firefox
> 14 (which worked perfectly for me), or did I get that wrong?

It would enable the whole accessibility which would cause to fail. *hint* it is *disabled* in Firefox 14.
There is definitely a bug, but setting AXEnhancedUserInterface would just be wrong.

> 
> There are many window resizing/moving tools out there and I think keeping
> the accessibility features turned off by default may result in quite a few
> complaining users when this goes live. (Especially because it has been
> working for such a long time now)

That is why I didn't mark the bug as WONTFIX.
Comment 16 Andreas H. 2012-08-17 15:38:31 PDT
ahh, thanks a lot for the clarification. I didn't get that.

Probably not important for fixing this but I wondered why Spectacle can't get the focused window and I can, so I checked their code. It looks like AXUIElementCopyAttributeValue() produces the error kAXErrorNoValue when Firefox is active and you try to get the kAXFocusedApplicationAttribute, but it still returns the valid AXWindow element. Their code only checks for kAXErrorSuccess and discards the returned AXWindow if there occurs any error.
Comment 17 Hubert Figuiere [:hub] 2012-08-17 18:10:51 PDT
Created attachment 653003 [details] [diff] [review]
Have minimum a11y support when we don't have Voice Over. r=
Comment 18 Hubert Figuiere [:hub] 2012-08-17 18:11:29 PDT
Comment on attachment 653003 [details] [diff] [review]
Have minimum a11y support when we don't have Voice Over. r=

Proposed patch: pass the a11y function if we don't have Voice Over.

Also fix a (begnin) issue is [ChildView accessible].

I also would like to propose it to Aurora to get this fixed as it is a regression.
Comment 19 David Bolter [:davidb] 2012-08-20 06:32:09 PDT
Comment on attachment 653003 [details] [diff] [review]
Have minimum a11y support when we don't have Voice Over. r=

Review of attachment 653003 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.

::: widget/cocoa/nsChildView.mm
@@ +4864,5 @@
>  - (BOOL)accessibilityIsIgnored
>  {
> +  if (!mozilla::a11y::ShouldA11yBeEnabled())
> +    return [super accessibilityIsIgnored];
> +  

nit: can delete spaces

@@ +4939,2 @@
>    id<mozAccessible> accessible = [self accessible];
>    

nit: space
Comment 20 Hubert Figuiere [:hub] 2012-08-20 07:20:31 PDT
If you want to give it a try, there is a build in progress for MacOS that will appear here when it is done:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/hfiguiere@mozilla.com-6347808ef957/

I tested with Spectacle but not with BetterSnapTool.
Comment 21 Andreas H. 2012-08-20 07:29:44 PDT
I just tested your build and it seems to work perfectly again. (BetterSnapTool, Spectacle, Accessibility Inspector)

Thanks a lot for fixing this so fast!
Comment 22 Hubert Figuiere [:hub] 2012-08-20 07:38:24 PDT
Comment on attachment 653003 [details] [diff] [review]
Have minimum a11y support when we don't have Voice Over. r=

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 781090
User impact if declined: Some window management tools won't work. (Mac only)
Testing completed (on m-c, etc.): tested locally. Reporter tested.
Risk to taking this patch (and alternatives if risky): a11y (Mac only)
String or UUID changes made by this patch: none.
Comment 24 Ed Morley [:emorley] 2012-08-21 06:32:07 PDT
https://hg.mozilla.org/mozilla-central/rev/75ed4a4cc102

Note You need to log in before you can comment on or make changes to this bug.