We can reuse some of the prompting logic I was planning on taking out in bug 1387507.

This is a prompt that does not provide a "more info" option. I'm not sure if we need more. One thing we could do if we went with an about page is provide a direct link to current esr downloads. I'm not sure adding all the code and protocol glue is worth that though. David, any thoughts?

Comment on attachment 8900869 [details] jaws prompt without about page.jpg I think what you have here is a decent option thanks Jim. (I'm not the best reviewer for the wording)

Comment on attachment 8900901 [details] [diff] [review] init compatibility info sooner Review of attachment 8900901 [details] [diff] [review]: ----------------------------------------------------------------- I think I miss the idea of the patch. The only one reason I can see for NOTINITIALIZED value is to show it in telemetry, but telemetry is collected only if ally is on. Could you please give more background for the changes? ::: accessible/base/nsAccessibilityService.cpp @@ +1262,5 @@ > return false; > > observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false); > > + // This information needs to be initialized before the observer fires. could you extend the commit to say why you have to call that before the observer fires. Also we used to initialize the compatibility in parent process only (PlatformInit). Why do we want to initialize it in content also?

(In reply to alexander :surkov from comment #7) > Comment on attachment 8900901 [details] [diff] [review] > init compatibility info sooner > > Review of attachment 8900901 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think I miss the idea of the patch. The only one reason I can see for > NOTINITIALIZED value is to show it in telemetry, but telemetry is collected > only if ally is on. Could you please give more background for the changes? > > ::: accessible/base/nsAccessibilityService.cpp > @@ +1262,5 @@ > > return false; > > > > observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false); > > > > + // This information needs to be initialized before the observer fires. > > could you extend the commit to say why you have to call that before the > observer fires. Sure I can add more here. If you look at the follow up patch you'll see why, the observer fires triggering ContentParent a11y initialization. I need JAWS info available in Compatibility so I can use the static jaws method to check for it's presence. > > Also we used to initialize the compatibility in parent process only > (PlatformInit). Why do we want to initialize it in content also? I have no need for this info in content. If this change triggers that I can avoid it with a process check.

Why don't you move Compatibility::Init from Platform::Init to a place where you need it?

(In reply to alexander :surkov from comment #9) > Why don't you move Compatibility::Init from Platform::Init to a place where > you need it? If I don't miss anything then you could move Compatibility::Init out PlatformInit, and then you don't need to introduce UNINITALIZED value. In case if there's a reason why you have to call Compatibility::Init twice, then it'd be nice to make UNINITALIZED = 0 and put it in beginning of enum. I'm pto next week, and I don't want to block you, so David is probably a good candidate for this.

Comment on attachment 8900901 [details] [diff] [review] init compatibility info sooner Review of attachment 8900901 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/base/nsAccessibilityService.cpp @@ +1263,5 @@ > > observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false); > > + // This information needs to be initialized before the observer fires. > + Compatibility::Init(); Compatibility is windows specific class, so if you have to have to call it here, then you should ifdefed it at least

Comment on attachment 8901886 [details] [diff] [review] init compatibility info sooner v.2 Eitan, would you mind picking this review up?

Comment on attachment 8901886 [details] [diff] [review] init compatibility info sooner v.2 Looks good!

Comment on attachment 8902256 [details] [diff] [review] notification patch v.1 Review of attachment 8902256 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/nsBrowserGlue.js @@ +1057,5 @@ > > this._sanitizer.onStartup(); > + > + if (AppConstants.platform == "win") { > + JawsScreenReaderVersionCheck.onWindowsRestored(); Can you add this into the _scheduleStartupIdleTasks function instead of here on OnWindowsRestored? @@ +2850,5 @@ > + Services.tm.dispatchToMainThread(() => this._checkVersionAndPrompt()); > + } > + }, > + > + onWindowsRestored() { I see that this was the pattern of the previous code that was removed, but I should ask just in case: Isn't there a chance that the a11y-init-or-shutdown notification fired earlier than the observer was added, in which case you'd want to unconditionally _checkVersionAndPrompt here, instead of doing that only if _wantsPrompt is true? @@ +2896,5 @@ > + notification.remove(); > + } > + }; > + let options = { > + popupIconURL: "chrome://browser/skin/e10s-64@2x.png", Ah, ok.. I had asked to remove this icon but I see that it's gonna be reused here @@ +2899,5 @@ > + let options = { > + popupIconURL: "chrome://browser/skin/e10s-64@2x.png", > + persistWhileVisible: true, > + persistent: true, > + persistence: 100 Reading the code in PopupNotifications I didn't understand the benefit of using `persistence` if `persistWhileVisible` is also used ::: browser/locales/en-US/chrome/browser/browser.properties @@ +787,5 @@ > > +# LOCALIZATION NOTE (e10s.accessibilityNotice.mainMessage3): %S is brandShortName > +e10s.accessibilityNotice.mainMessage3 = Display of tab content is disabled due to incompatibility between %S and your accessibility software. Please update your screen reader or switch to Firefox Extended Support Release. > +e10s.accessibilityNotice.acceptButton.label = OK > +e10s.accessibilityNotice.acceptButton.accesskey = O let's not remove this label/accesskey on the other bug and re-add it here.. I think this might cause issues for the l10n tools..

Jimm mentioned this bug fix will likely land in 57 as per the e10s/a11y plans

(In reply to :Felipe Gomes (needinfo me!) from comment #16) > > this._sanitizer.onStartup(); > > + > > + if (AppConstants.platform == "win") { > > + JawsScreenReaderVersionCheck.onWindowsRestored(); > > Can you add this into the _scheduleStartupIdleTasks function instead of here > on OnWindowsRestored? updated. > @@ +2850,5 @@ > > + Services.tm.dispatchToMainThread(() => this._checkVersionAndPrompt()); > > + } > > + }, > > + > > + onWindowsRestored() { > > I see that this was the pattern of the previous code that was removed, but I > should ask just in case: Isn't there a chance that the a11y-init-or-shutdown > notification fired earlier than the observer was added, in which case you'd > want to unconditionally _checkVersionAndPrompt here, instead of doing that > only if _wantsPrompt is true? I've simplified this quite a bit. scheduleStartupIdleTasks insures this only gets called once. > > @@ +2896,5 @@ > > + notification.remove(); > > + } > > + }; > > + let options = { > > + popupIconURL: "chrome://browser/skin/e10s-64@2x.png", > > Ah, ok.. I had asked to remove this icon but I see that it's gonna be reused removed from the parent bug patches, added back here. > @@ +2899,5 @@ > > + let options = { > > + popupIconURL: "chrome://browser/skin/e10s-64@2x.png", > > + persistWhileVisible: true, > > + persistent: true, > > + persistence: 100 > > Reading the code in PopupNotifications I didn't understand the benefit of > using `persistence` if `persistWhileVisible` is also used This popup code is crazy, take a look here - http://searchfox.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#524 AFAICT you need 'persistWhileVisible' to pass the checks on navigation here, and you also need 'persistence' to be a positive value. You also need 'persistent' set to avoid having auto-hide features enabled. Testing here I couldn't get this notification to stay up past the first navigation without all of these values set. > > ::: browser/locales/en-US/chrome/browser/browser.properties > @@ +787,5 @@ > > > > +# LOCALIZATION NOTE (e10s.accessibilityNotice.mainMessage3): %S is brandShortName > > +e10s.accessibilityNotice.mainMessage3 = Display of tab content is disabled due to incompatibility between %S and your accessibility software. Please update your screen reader or switch to Firefox Extended Support Release. > > +e10s.accessibilityNotice.acceptButton.label = OK > > +e10s.accessibilityNotice.acceptButton.accesskey = O > > let's not remove this label/accesskey on the other bug and re-add it here.. > I think this might cause issues for the l10n tools.. Kept all strings from the previous patches and added a new one for this notification.

Pushed by jmathies@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b204be1e64 Initialize accessibility compaitibility information earlier in accessibility service startup. r=eitan https://hg.mozilla.org/integration/mozilla-inbound/rev/4622489f9872 Prevent initialization of content accessibility when older versions of the JAWS screen reader are detected. r=surkov https://hg.mozilla.org/integration/mozilla-inbound/rev/94b2d9b60f8c Provide a chrome side notification informing the user about an incompatible version of JAWS screen reader. r=felipe

https://hg.mozilla.org/mozilla-central/rev/f0b204be1e64 https://hg.mozilla.org/mozilla-central/rev/4622489f9872 https://hg.mozilla.org/mozilla-central/rev/94b2d9b60f8c

I think this is what's needed for 56, stolen from bug 1385991.

Comment on attachment 8911875 [details] [diff] [review] patch for 56 release Review of attachment 8911875 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Julien Cristau [:jcristau] from comment #24) > Created attachment 8911875 [details] [diff] [review] > patch for 56 release > > I think this is what's needed for 56, stolen from bug 1385991. Yep this looks good. I found the code I was looking for in bug 1391733. That's actually in 56, which is why I thought shouldBlockIncompatJaws was already available there. Didn't realize we landed the app info boolean on to 57.

Comment on attachment 8911875 [details] [diff] [review] patch for 56 release Approval Request Comment [Feature/Bug causing the regression]: blocking jaws users from updating in 56 [User impact if declined]: can't decline this [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: yes [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: new interface to an understood value. [String changes made/needed]:

Comment on attachment 8911875 [details] [diff] [review] patch for 56 release Does this need uplift to 57 (beta)? I'm still not clear on that. But let's bring this to release and verify once it lands

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #27) > Comment on attachment 8911875 [details] [diff] [review] > patch for 56 release > > Does this need uplift to 57 (beta)? I'm still not clear on that. But let's > bring this to release and verify once it lands Marked fixed in 57 so if beta is 57 then we're ok.

Landed the JAWS detection code on 56. https://hg.mozilla.org/releases/mozilla-release/rev/8c340d0042b510bf984437ca3e5bbbe06daf95d4

This is verified fixed in 56.0.2 and 57.b12.