Closed
Bug 811307
Opened 13 years ago
Closed 12 years ago
[AccessFu] Add mochitest for enabling
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: davidb, Assigned: yzen)
Details
Attachments
(2 files, 13 obsolete files)
14.32 KB,
patch
|
eeejay
:
review+
|
Details | Diff | Splinter Review |
8.53 KB,
patch
|
MarcoZ
:
review+
|
Details | Diff | Splinter Review |
Breaking the ice on AccessFu (jsat) mochitests.
Reporter | ||
Comment 1•13 years ago
|
||
Assignee: nobody → dbolter
Attachment #681040 -
Flags: review?(eitan)
Attachment #681040 -
Flags: feedback?(marco.zehe)
Reporter | ||
Comment 2•13 years ago
|
||
Comment on attachment 681040 [details] [diff] [review]
patch
Review of attachment 681040 [details] [diff] [review]:
-----------------------------------------------------------------
::: accessible/tests/mochitest/jsat/test_alive.html
@@ +58,5 @@
> +
> + <a target="_blank"
> + href="https://bugzilla.mozilla.org/show_bug.cgi?id=??????"
> + title="Add prelimenary jsat (AccessFu) support">
> + Mozilla Bug ??????
I need to update this of course.
Comment 3•13 years ago
|
||
Comment on attachment 681040 [details] [diff] [review]
patch
Please change the title of the test_alive.html file. ;)
Other than that, this looks correct to me. Even this monstrous call in utils.js. ;)
Attachment #681040 -
Flags: feedback?(marco.zehe) → feedback+
Reporter | ||
Comment 4•13 years ago
|
||
(In reply to Marco Zehe (:MarcoZ) from comment #3)
> Comment on attachment 681040 [details] [diff] [review]
> patch
>
> Please change the title of the test_alive.html file. ;)
Good catch. Fixed locally.
Comment 5•13 years ago
|
||
Comment on attachment 681040 [details] [diff] [review]
patch
Review of attachment 681040 [details] [diff] [review]:
-----------------------------------------------------------------
This is good. Pending on a new place to put the top level window getter.
::: accessible/src/jsat/AccessFu.jsm
@@ +99,5 @@
> this.chromeWin.addEventListener('TabOpen', this);
> this.chromeWin.addEventListener('TabSelect', this);
> +
> + if (this.readyCallback)
> + this.readyCallback();
Nice. This is low-profile, and doesn't introduce more API complexity.
::: accessible/tests/mochitest/jsat/test_alive.html
@@ +36,5 @@
> +
> + AccessFu.readyCallback = function readyCallback() {
> + gAccessFuStarted = true;
> + }
> +
Trailing whitespace. Do we care about that?
::: accessible/tests/mochitest/utils.js
@@ +1,4 @@
> +/**
> + * Return main window (crosses chrome boundary)
> + */
> + function getMainWindow(aWindow)
The name should probably indicate top-level and chrome, maybe getTopChromeWindow?
Seems excessive to start a new utils file for this. What about dropping it in common.js, or have a jsat_common.js since we will probably need this in all jsat tests.
Reporter | ||
Comment 6•13 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #5)
> Comment on attachment 681040 [details] [diff] [review]
> patch
>
> Review of attachment 681040 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> This is good. Pending on a new place to put the top level window getter.
>
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +99,5 @@
> > this.chromeWin.addEventListener('TabOpen', this);
> > this.chromeWin.addEventListener('TabSelect', this);
> > +
> > + if (this.readyCallback)
> > + this.readyCallback();
>
> Nice. This is low-profile, and doesn't introduce more API complexity.
Thanks.
>
> ::: accessible/tests/mochitest/jsat/test_alive.html
> @@ +36,5 @@
> > +
> > + AccessFu.readyCallback = function readyCallback() {
> > + gAccessFuStarted = true;
> > + }
> > +
>
> Trailing whitespace. Do we care about that?
>
> ::: accessible/tests/mochitest/utils.js
> @@ +1,4 @@
> > +/**
> > + * Return main window (crosses chrome boundary)
> > + */
> > + function getMainWindow(aWindow)
>
> The name should probably indicate top-level and chrome, maybe
> getTopChromeWindow?
Sure I can make that change. I had been biased with the variable name "mainWindow" in the related wiki:
https://developer.mozilla.org/en/docs/Working_with_windows_in_chrome_code
>
> Seems excessive to start a new utils file for this. What about dropping it
> in common.js, or have a jsat_common.js since we will probably need this in
> all jsat tests.
I originally had it in common.js... I'll move it back there.
Reporter | ||
Comment 7•13 years ago
|
||
Attachment #681040 -
Attachment is obsolete: true
Attachment #681040 -
Flags: review?(eitan)
Attachment #681130 -
Flags: review?(eitan)
Comment 8•13 years ago
|
||
Comment on attachment 681130 [details] [diff] [review]
patch2
Review of attachment 681130 [details] [diff] [review]:
-----------------------------------------------------------------
Perfect.
Attachment #681130 -
Flags: review?(eitan) → review+
Reporter | ||
Comment 9•13 years ago
|
||
Oof forgot a final qrefresh. I went with your naming suggestion so even more perfect!
Reporter | ||
Comment 10•13 years ago
|
||
Reporter | ||
Comment 11•13 years ago
|
||
Hmmm, I might need to turn AccessFu off at the end of the test otherwise it will be actively running for the remainder of our test presumably.
Reporter | ||
Comment 12•13 years ago
|
||
Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5324de8c398
Interestingly this causes perma-oranges on some otherwise intermittent oranges (same thing happened on try). I'll investigate before relanding.
Comment 13•12 years ago
|
||
Comment on attachment 681130 [details] [diff] [review]
patch2
Review of attachment 681130 [details] [diff] [review]:
-----------------------------------------------------------------
Re-reviewing in light of the orange. This is likely a memory leak. Bug #850005 should resolve the deep AccessFu leak issues. There is one issue here below.
::: accessible/src/jsat/AccessFu.jsm
@@ +99,5 @@
> this.chromeWin.addEventListener('TabOpen', this);
> this.chromeWin.addEventListener('TabSelect', this);
> +
> + if (this.readyCallback)
> + this.readyCallback();
AccessFu.readyCallback should be deleted after it is called. since it has an indirect reference to a window object.
Attachment #681130 -
Flags: review+ → review-
Comment 14•12 years ago
|
||
Comment 15•12 years ago
|
||
Try run for 16bcd9599226 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=16bcd9599226
Results (out of 387 total builds):
exception: 6
success: 330
warnings: 46
failure: 4
other: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eisaacson@mozilla.com-16bcd9599226
Comment 16•12 years ago
|
||
From that try run, all a11y mochitests failed with bug 781120.
Reporter | ||
Comment 17•12 years ago
|
||
I wonder why... ?
Comment 18•12 years ago
|
||
The debug runs also showed a leak. Are we sure this try run included the patch from bug 850005?
Comment 19•12 years ago
|
||
Ok. Lets keep the r- on that patch. There are two things that are missing:
1. A teardown. AccessFu needs to be disabled. If not, it keeps capturing keystrokes and other things.
2. Calling attach() before setting the pref would be a lot more interesting, since we would be testing that AccessFu gets enabled when the pref changes.
Bonus points: test for the case when the activate pref is 2 (auto). That means adding a special test mode, and listening for the Android "Accessibility:Settings" notification.
Comment 20•12 years ago
|
||
Try run for 2a90f51410a2 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=2a90f51410a2
Results (out of 19 total builds):
exception: 18
success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mzehe@mozilla.com-2a90f51410a2
Comment 21•12 years ago
|
||
This is a stab at a tear-down. However, the try run I kicked off gives me the following error:
04:07:30 INFO - 5002 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/jsat/test_alive.html | uncaught exception - Error: Only one top-level window could used with AccessFu at resource://gre/modules/accessibility/Utils.jsm:26
What's the best way to tear AccessFu down?
Comment 22•12 years ago
|
||
I tried taking a stab at the tear-down, but I encountered this error on the try run:
04:07:30 INFO - 5002 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/jsat/test_alive.html | uncaught exception - Error: Only one top-level window could used with AccessFu at resource://gre/modules/accessibility/Utils.jsm:26
Looks like it doesn't take kindly to just switching the pref from 1 to 0 between calls and hoping it'll just work. :)
Eitan, any hints on what to do to best tear it down after the test?
Updated•12 years ago
|
Attachment #726588 -
Attachment is obsolete: true
Comment 23•12 years ago
|
||
Try run for feeeff98d470 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=feeeff98d470
Results (out of 381 total builds):
exception: 1
success: 340
warnings: 34
failure: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mzehe@mozilla.com-feeeff98d470
Comment 24•12 years ago
|
||
This patch works, it starts and stops AccessFu and logs both instances.
Attachment #726706 -
Flags: review?(eitan)
Updated•12 years ago
|
Attachment #726615 -
Attachment is obsolete: true
Comment 25•12 years ago
|
||
(In reply to Marco Zehe (:MarcoZ) from comment #24)
> Created attachment 726706 [details] [diff] [review]
> [AccessFu] Add mochitest for enabling. Tear-down bits by MarcoZ.
>
> This patch works, it starts and stops AccessFu and logs both instances.
Is this the same patch you put in the previous try?
https://tbpl.mozilla.org/?tree=Try&rev=feeeff98d470
If so, I still see a lot of orange.
Comment 26•12 years ago
|
||
(In reply to Marco Zehe (:MarcoZ) from comment #22)
> Created attachment 726615 [details] [diff] [review]
> [AccessFu] Add mochitest for enabling. Tear-down bits by MarcoZ.
>
> I tried taking a stab at the tear-down, but I encountered this error on the
> try run:
>
> 04:07:30 INFO - 5002 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/a11y/accessible/jsat/test_alive.html | uncaught
> exception - Error: Only one top-level window could used with AccessFu at
> resource://gre/modules/accessibility/Utils.jsm:26
>
> Looks like it doesn't take kindly to just switching the pref from 1 to 0
> between calls and hoping it'll just work. :)
This is because it is done asynchronously. You set the pref and then the observer is fired later. So you need to chain everything in the callbacks. In other words, in the ready call, you continue the test, and in the done callback you explicitly finish the test.
> Eitan, any hints on what to do to best tear it down after the test?
As a failsafe, I would also do that in document unload.
Comment 27•12 years ago
|
||
Comment on attachment 726706 [details] [diff] [review]
[AccessFu] Add mochitest for enabling. Tear-down bits by MarcoZ.
Review of attachment 726706 [details] [diff] [review]:
-----------------------------------------------------------------
Like I said above, using the pref should be possible. It is asynchronous, so the test needs to be chained in the various callbacks.
::: accessible/src/jsat/AccessFu.jsm
@@ +62,5 @@
> + * Exposes the ability to shut down AccessFu from the outside.
> + */
> + detach: function detach() {
> + this._disable();
> + },
We should not enable/disable directly, but only via pref.
A detach method might be needed in the future for actually disassociating the window, but it would need to do more than just disabling, it would need to remove observers and event handlers that are set up in attach().
Attachment #726706 -
Flags: review?(eitan) → review-
Comment 28•12 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #27)
>
> We should not enable/disable directly, but only via pref.
>
To clarify, doing it directly is a bad idea because you would be putting AccessFu into a compromised state where the pref says one thing, but AccessFu is in a different state.
Reporter | ||
Comment 29•12 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #28)
> (In reply to Eitan Isaacson [:eeejay] from comment #27)
> >
> > We should not enable/disable directly, but only via pref.
> >
>
> To clarify, doing it directly is a bad idea because you would be putting
> AccessFu into a compromised state where the pref says one thing, but
> AccessFu is in a different state.
OK
Comment 30•12 years ago
|
||
Eitan, the patch I put here was not the one I did the try run with, I did that and then managed to run the tests locally, too, and fix it up that way. I will take your comments into account and put up a new patch tomorrow. That is, if David allows me to continue stealing the bug. ;-)
Reporter | ||
Comment 31•12 years ago
|
||
Steal away! Eitan has a good idea of how this can be nice. Please chat with him first.
Comment 32•12 years ago
|
||
(In reply to Marco Zehe (:MarcoZ) from comment #30)
> Eitan, the patch I put here was not the one I did the try run with, I did
> that and then managed to run the tests locally, too, and fix it up that way.
> I will take your comments into account and put up a new patch tomorrow. That
> is, if David allows me to continue stealing the bug. ;-)
I think the goal of this patch should be having a common bomb-proof setup/teardown for this and future accessfu tests. The way a test should be run is like this:
AccessFuTest.addTest(testMe);
AccessFuTest.run();
Where the function testMe should be a specific test.
AccessfuTest.run() should:
1. do AccessFu.attach()
2. Set a ready callback (see below)
3. set the activate pref to 1
The ready callback should:
1. iterate over added tests and run them safely (in try/catch).
2. set the done callback (see below).
3. set the activate pref to 0
the done callback should:
1. detach AccessFu with a new detach method that would unregister all the observers and reverse everything attach() does.
2. call SimpleTest.finish()
AccessFuTest should live in a new common js file in the jsat test folder so all future AccessFu tests could use it.
Sorry if this is too specific. Please take creative control and have fun with it. As long as it answers the goal above.
Reporter | ||
Updated•12 years ago
|
Assignee: dbolter → marco.zehe
Reporter | ||
Comment 33•12 years ago
|
||
Marco any luck?
Comment 34•12 years ago
|
||
Dumping this here for safe-keeping. It currently gives an error message at tear-down that aWindow.shell isn't defined. It also starts the AccessFu event manager after everything has already shut down again, at least according to the log I get. Nightly hangs around with or without the larger AccessFu.detach than in the previous patch, and if I only have the _disable() call in there, eventManager keeps interfering with future tests and does all kinds of stupid stuff. I currently don't find where this thing is getting started and how to properly tear it down. Comments appreciated, otherwise I'll also continue banging on this tomorrow.
Updated•12 years ago
|
Attachment #726706 -
Attachment is obsolete: true
Assignee | ||
Comment 35•12 years ago
|
||
(In reply to Marco Zehe (:MarcoZ) from comment #34)
> Created attachment 734659 [details] [diff] [review]
> [AccessFu] Add mochitest for enabling. Tear-down bits by MarcoZ.
>
> Dumping this here for safe-keeping. It currently gives an error message at
> tear-down that aWindow.shell isn't defined. It also starts the AccessFu
> event manager after everything has already shut down again, at least
> according to the log I get. Nightly hangs around with or without the larger
> AccessFu.detach than in the previous patch, and if I only have the
> _disable() call in there, eventManager keeps interfering with future tests
> and does all kinds of stupid stuff. I currently don't find where this thing
> is getting started and how to properly tear it down. Comments appreciated,
> otherwise I'll also continue banging on this tomorrow.
Hi Marco,
I've taken a look at the patch and here are my observations:
* aWindow.shell is only present in b2g and I believe this is why you are seeing that shell is undefined error.
* When you are removing a listener to 'ContentStart' event in detach, you need to provide the original listener object as well.
* Another thing I noticed, but not sure if it is necessary, is that if you want to truly detach AccessFu I think you would need to clear the reference to aWindow object created by Utils.init.
* Also, I tried checking if AccessFu._enabled is true to avoid disabling twice.
After the above changes I was able to run the test successfully.
Let me know if it works for you :)
Comment 36•12 years ago
|
||
Yura, would you like to take this bug and apply your changes to the patch? I'd be happy to give feedback or review on it. You seem to have a much better grasp on these intricacies than I do. :)
Assignee | ||
Updated•12 years ago
|
Assignee: marco.zehe → yura.zenevich
Assignee | ||
Comment 37•12 years ago
|
||
Hi Marco,
I got to a point where I have a nice looking detach that works properly however I ran into a problem:
AccessFu has a method _loadFrameScript that is called in _enable for every message manager. It is also called on 'remote-browser-frame-shown' and 'TabOpen' events.
The method adds some message listeners that could be removed on disable however it also calls loadFrameScript for 'content-script.js'. This means that we are loading 'content-script.js' every time we enable.. As far as I understand, this process is irreversible and thus should only happen once. Would you know if that's the case? Perhaps you have any suggestions as to how to go around this issue?
Thanks.
Flags: needinfo?(marco.zehe)
Comment 38•12 years ago
|
||
Phew, I'm afraid this is a little beyond my knowledge of the intricacies of the out-of-process workings. Deferring to Eitan who is on PTO this week, so we may not get an answer until next week.
But it definitely has to do with the way we have to deal with separate content processes in Firefox OS and possibly Firefox for Android. On Windows, Linux and Mac, OOP is currently not supported and has been postponed for an unknown time.
Flags: needinfo?(marco.zehe) → needinfo?(eitan)
Assignee | ||
Comment 39•12 years ago
|
||
This now passes. However, I think a more reliable test would be listening for some definitive event such as "AccessFu:Start" or some other one like that.
Attachment #736122 -
Flags: feedback?(marco.zehe)
Attachment #736122 -
Flags: feedback?(eitan)
Assignee | ||
Comment 40•12 years ago
|
||
These are all the changes I had to make to make sure that AccessFu gets disabled and detached cleanly.
I also feel like done and ready and done callbacks should, perhaps, be handled through some sort of an event mechanism.
Attachment #736123 -
Flags: feedback?(marco.zehe)
Attachment #736123 -
Flags: feedback?(eitan)
Comment 41•12 years ago
|
||
Comment on attachment 736122 [details] [diff] [review]
Slightly updated test by Marco.
>+ var aWindow = getMainChromeWindow(window);
>+
>+ AccessFu.attach(aWindow);
Why this change? You don't appear to use the aWindow variable anywhere else.
Also, aWindow should be named slightly differently, since by convention, aWindow would indicate a parameter passed, not a local variable. I'd call it localWin or chromeWin or something similar.
Comment 42•12 years ago
|
||
Comment on attachment 736123 [details] [diff] [review]
Changes to AccessFu to enable testing.
>+ _handleMessageManager: function _handleMessageManager(aMessageManager) {
>+ if (this._enabled) {
>+ this._addMessageListeners(mm);
>+ }
>+ this._loadFrameScript(mm);
> },
You meant to write "aMessageManager" instead of "mm", right? Does zour patch still work with this change?
Assignee | ||
Comment 43•12 years ago
|
||
(In reply to Marco Zehe (:MarcoZ) from comment #41)
> Comment on attachment 736122 [details] [diff] [review]
> Slightly updated test by Marco.
>
> >+ var aWindow = getMainChromeWindow(window);
> >+
> >+ AccessFu.attach(aWindow);
>
> Why this change? You don't appear to use the aWindow variable anywhere else.
>
> Also, aWindow should be named slightly differently, since by convention,
> aWindow would indicate a parameter passed, not a local variable. I'd call it
> localWin or chromeWin or something similar.
Ah, good point, when I was created it getMainChromeWindow was used twice and then I removed the second case. Thanks!
Assignee | ||
Comment 44•12 years ago
|
||
(In reply to Marco Zehe (:MarcoZ) from comment #42)
> Comment on attachment 736123 [details] [diff] [review]
> Changes to AccessFu to enable testing.
>
> >+ _handleMessageManager: function _handleMessageManager(aMessageManager) {
> >+ if (this._enabled) {
> >+ this._addMessageListeners(mm);
> >+ }
> >+ this._loadFrameScript(mm);
> > },
>
> You meant to write "aMessageManager" instead of "mm", right? Does zour patch
> still work with this change?
Thanks for catching that, yes it still worked event with this typo :) I'll update the patches.
Assignee | ||
Comment 45•12 years ago
|
||
Attachment #736122 -
Attachment is obsolete: true
Attachment #736122 -
Flags: feedback?(marco.zehe)
Attachment #736122 -
Flags: feedback?(eitan)
Attachment #736260 -
Flags: feedback?(marco.zehe)
Attachment #736260 -
Flags: feedback?(eitan)
Assignee | ||
Comment 46•12 years ago
|
||
Attachment #736123 -
Attachment is obsolete: true
Attachment #736123 -
Flags: feedback?(marco.zehe)
Attachment #736123 -
Flags: feedback?(eitan)
Attachment #736262 -
Flags: feedback?(marco.zehe)
Attachment #736262 -
Flags: feedback?(eitan)
Comment 47•12 years ago
|
||
Comment on attachment 736260 [details] [diff] [review]
Latest test.
This looks good to me. Would be interesting to have a try-server run to see how these changes fare on different platforms in production conditions.
Attachment #736260 -
Flags: feedback?(marco.zehe) → feedback+
Comment 48•12 years ago
|
||
Comment on attachment 736262 [details] [diff] [review]
Latest changes to AccessFu to enable testing.
These changes look good to me as well, I also tested a local build with these patches and found that everything still works as expected so far.
Attachment #736262 -
Flags: feedback?(marco.zehe) → feedback+
Comment 49•12 years ago
|
||
Comment on attachment 736260 [details] [diff] [review]
Latest test.
Review of attachment 736260 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good.
Attachment #736260 -
Flags: feedback?(eitan) → feedback+
Flags: needinfo?(eitan)
Comment 50•12 years ago
|
||
Comment on attachment 736262 [details] [diff] [review]
Latest changes to AccessFu to enable testing.
Review of attachment 736262 [details] [diff] [review]:
-----------------------------------------------------------------
This looks pretty good. I'd like to see it tested on Android by disabling and enabling TalkBack and playing with the pref.
::: accessible/src/jsat/AccessFu.jsm
@@ +42,5 @@
> + if (Utils.MozBuildApp === 'b2g') {
> + this._contentStartListener = function _contentStartListener() {
> + aWindow.shell.contentBrowser.contentWindow.addEventListener(
> + 'mozContentEvent', this, false, true);
> + };
I think you need to bind the function to "this" or else "this" that is in the function is not AccessFu.
@@ +44,5 @@
> + aWindow.shell.contentBrowser.contentWindow.addEventListener(
> + 'mozContentEvent', this, false, true);
> + };
> + aWindow.addEventListener('ContentStart', this._contentStartListener,
> + false);
Why not just make AccessFu the listener object instead of this._contentStartListener?
Attachment #736262 -
Flags: feedback?(eitan) → feedback+
Assignee | ||
Comment 51•12 years ago
|
||
Attachment #736262 -
Attachment is obsolete: true
Attachment #738539 -
Flags: review?(eitan)
Assignee | ||
Comment 52•12 years ago
|
||
Attachment #736260 -
Attachment is obsolete: true
Attachment #738543 -
Flags: review?(marco.zehe)
Updated•12 years ago
|
Attachment #734659 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #738543 -
Flags: review?(marco.zehe) → review+
Comment 53•12 years ago
|
||
Comment on attachment 738539 [details] [diff] [review]
Patch with comments addressed.
Review of attachment 738539 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good. A few thing need to happen before this lands:
1. A successful try run (for this and the other patch).
2. Testing on Android for the following cases without restarting firefox:
a. activate pref set to 0: accessfu disabled both with system a11y on or off.
b. activate pref set to 1: accessfu enabled both with system a11y on or off.
c. activate pref set to 2: accessfu enabled/disabled depending on system pref.
Attachment #738539 -
Flags: review?(eitan) → review+
Comment 54•12 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #53)
> This looks good. A few thing need to happen before this lands:
> 1. A successful try run (for this and the other patch).
I just started that. Results will be posted to this bug once completed, but if you'd like to watch, the link is:
https://tbpl.mozilla.org/?tree=Try&rev=466f4d6dcadd
> 2. Testing on Android for the following cases without restarting firefox:
> a. activate pref set to 0: accessfu disabled both with system a11y on or
> off.
> b. activate pref set to 1: accessfu enabled both with system a11y on or off.
> c. activate pref set to 2: accessfu enabled/disabled depending on system
> pref.
I definitely need this to be done by a sighted person, since parts of this involve the accessibility leaving a blind guy in the dark on purpose. :)
Assignee | ||
Comment 55•12 years ago
|
||
After doing some investigation why AccessFu would not resume on an already opened page on Android, after being disabled and then activated again, I found that the EventManager would not start after AccessFu gets enabled. Here's the log:
...
I/Gecko (17143): [AccessFu] INFO disable
I/Gecko (17143): [AccessFu] INFO TouchAdapter.stop
I/Gecko (17143): [AccessFu] INFO EventManager.stop mobile/android
I/Gecko (17143): [AccessFu] INFO enable
I/Gecko (17143): [AccessFu] INFO TouchAdapter.start
...
I think it's possible to write a test for it, so I'll try to do that and fix the issue.
Reporter | ||
Comment 56•12 years ago
|
||
(In reply to Yura Zenevich [:yzen] from comment #55)
> I think it's possible to write a test for it, so I'll try to do that and fix
> the issue.
<3
Assignee | ||
Comment 57•12 years ago
|
||
Fixed the issue related to AccessFu:start message. Will push to try and try the build on device.
Attachment #738539 -
Attachment is obsolete: true
Attachment #739428 -
Flags: feedback?(eitan)
Assignee | ||
Comment 58•12 years ago
|
||
Attachment #738543 -
Attachment is obsolete: true
Attachment #739429 -
Flags: feedback?(marco.zehe)
Comment 59•12 years ago
|
||
Comment on attachment 739429 [details] [diff] [review]
Tests v4
> + // and finish the test run.
> + SimpleTest.finish();
> + };
Why no return; here any more?
> + waitForExplicitFinish: function AccessFuTest_waitForExplicitFinish() {
> + this._waitForExplicitFinish = true;
> + },
Where is it guaranteed that this is set to "false" initially, and not have some random value in the case the test file doesn't call waitForExplicitFinish()?
The tests look good otherwise, but I'd like these two above points answered, please.
Comment 60•12 years ago
|
||
Comment on attachment 739429 [details] [diff] [review]
Tests v4
We cleared the questions over IRC. Changing the feedback to a formal review to save a step. :) r=me.
Attachment #739429 -
Flags: feedback?(marco.zehe) → review+
Comment 61•12 years ago
|
||
Comment on attachment 739428 [details] [diff] [review]
Enabling testing for AccessFu v4
Review of attachment 739428 [details] [diff] [review]:
-----------------------------------------------------------------
r=me
Attachment #739428 -
Flags: feedback?(eitan) → review+
Assignee | ||
Comment 62•12 years ago
|
||
Attachment #739429 -
Attachment is obsolete: true
Attachment #739641 -
Flags: review?(marco.zehe)
Attachment #739641 -
Flags: feedback?(eitan)
Assignee | ||
Comment 63•12 years ago
|
||
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=6c4837273537
Assignee | ||
Comment 64•12 years ago
|
||
Build from try is available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yura.zenevich@gmail.com-6c4837273537/try-android-armv6/fennec-23.0a1.en-US.android-arm-armv6.apk
I tested it by setting accessfu.activate to 0, 1 and 2 and turning talk back on and off, it seems to behave correctly.
Comment 65•12 years ago
|
||
Comment on attachment 739641 [details] [diff] [review]
Tests v5
Nice! r=me.
Attachment #739641 -
Flags: review?(marco.zehe) → review+
Updated•12 years ago
|
Attachment #681130 -
Attachment is obsolete: true
Comment 66•12 years ago
|
||
Comment 67•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6915e00b6d62
https://hg.mozilla.org/mozilla-central/rev/8f39ea436f1d
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•12 years ago
|
Attachment #739641 -
Flags: feedback?(eitan)
You need to log in
before you can comment on or make changes to this bug.
Description
•