Closed Bug 1385991 Opened 7 years ago Closed 7 years ago

Detect and block older versions of JAWS that are not e10s compatible

Categories

(Core :: Disability Access APIs, defect, P1)

Unspecified
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox56 blocking verified
firefox57 + verified

People

(Reporter: bugzilla, Assigned: jimm)

References

Details

(Whiteboard: aes+)

Attachments

(5 files, 4 obsolete files)

      No description provided.
We can reuse some of the prompting logic I was planning on taking out in bug 1387507.
Blocks: 1387507
Assignee: nobody → jmathies
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?
Attachment #8900869 - Flags: feedback?(dbolter)
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)
Attachment #8900869 - Flags: feedback?(dbolter) → feedback+
Attached patch init compatibility info sooner (obsolete) — Splinter Review
Attachment #8900901 - Flags: review?(surkov.alexander)
Attachment #8900901 - Attachment is patch: true
Attachment #8900902 - Flags: review?(surkov.alexander)
No longer blocks: 1387507
Depends on: 1387507
Attached patch notification patch (obsolete) — Splinter Review
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?
Attachment #8900902 - Flags: review?(surkov.alexander) → review+
Blocks: 1393987
(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
Attachment #8900901 - Flags: review?(surkov.alexander)
Comment on attachment 8901886 [details] [diff] [review]
init compatibility info sooner v.2

Eitan, would you mind picking this review up?
Attachment #8901886 - Flags: review?(eitan)
No longer blocks: 1393987
Comment on attachment 8901886 [details] [diff] [review]
init compatibility info sooner v.2

Looks good!
Attachment #8901886 - Flags: review?(eitan) → review+
Attached patch notification patch v.1 (obsolete) — Splinter Review
Attachment #8900905 - Attachment is obsolete: true
Attachment #8902256 - Flags: review?(felipc)
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..
Attachment #8902256 - Flags: review?(felipc)
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.
Attached patch notification patch v.2 (obsolete) — Splinter Review
Attachment #8902256 - Attachment is obsolete: true
Attachment #8905936 - Attachment is obsolete: true
Attachment #8905940 - Flags: review?(felipc)
Attachment #8905940 - Flags: review?(felipc) → review+
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
Depends on: 1401561
I think this is what's needed for 56, stolen from bug 1385991.
Attachment #8911875 - Flags: review?(jmathies)
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.
Attachment #8911875 - Flags: review?(jmathies) → review+
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]:
Attachment #8911875 - Flags: approval-mozilla-release?
Attachment #8911875 - Flags: approval-mozilla-beta?
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
Attachment #8911875 - Flags: approval-mozilla-release? → approval-mozilla-release+
(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.
Attachment #8911875 - Flags: approval-mozilla-beta?
Flags: qe-verify+
This is verified fixed in 56.0.2 and 57.b12.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.