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

VERIFIED FIXED in Firefox 56

Status

()

P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: aklotz, Assigned: jimm)

Tracking

Trunk
mozilla57
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56blocking verified, firefox57+ verified)

Details

(Whiteboard: aes+)

Attachments

(5 attachments, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
We can reuse some of the prompting logic I was planning on taking out in bug 1387507.
Blocks: 1387507
(Assignee)

Updated

a year ago
Assignee: nobody → jmathies
(Assignee)

Comment 2

a year ago
Created attachment 8900869 [details]
jaws prompt without about page.jpg

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+
(Assignee)

Comment 4

a year ago
Created attachment 8900901 [details] [diff] [review]
init compatibility info sooner
Attachment #8900901 - Flags: review?(surkov.alexander)
(Assignee)

Updated

a year ago
Attachment #8900901 - Attachment is patch: true
(Assignee)

Comment 5

a year ago
Created attachment 8900902 [details] [diff] [review]
don't init if jaws is present
Attachment #8900902 - Flags: review?(surkov.alexander)
(Assignee)

Updated

a year ago
No longer blocks: 1387507
Depends on: 1387507
(Assignee)

Comment 6

a year ago
Created attachment 8900905 [details] [diff] [review]
notification patch
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?
(Assignee)

Comment 8

a year ago
(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+
(Assignee)

Updated

a year ago
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)
(Assignee)

Comment 13

a year ago
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)
(Assignee)

Updated

a year ago
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+
(Assignee)

Comment 15

a year ago
Created attachment 8902256 [details] [diff] [review]
notification patch v.1
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
status-firefox57: --- → affected
tracking-firefox57: --- → +
(Assignee)

Comment 18

a year ago
(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.
(Assignee)

Comment 19

a year ago
Created attachment 8905936 [details] [diff] [review]
notification patch v.2
Attachment #8902256 - Attachment is obsolete: true
(Assignee)

Comment 20

a year ago
Created attachment 8905940 [details] [diff] [review]
notification patch v.2
Attachment #8905936 - Attachment is obsolete: true
Attachment #8905940 - Flags: review?(felipc)
Attachment #8905940 - Flags: review?(felipc) → review+

Comment 22

a year ago
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
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Updated

a year ago
Depends on: 1401561
status-firefox56: --- → affected
tracking-firefox56: --- → blocking
Created attachment 8911875 [details] [diff] [review]
patch for 56 release

I think this is what's needed for 56, stolen from bug 1385991.
Attachment #8911875 - Flags: review?(jmathies)
(Assignee)

Comment 25

a year ago
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+
(Assignee)

Comment 26

a year ago
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+
(Assignee)

Comment 28

a year ago
(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.

Comment 29

a year ago
uplift
Landed the JAWS detection code on 56.
https://hg.mozilla.org/releases/mozilla-release/rev/8c340d0042b510bf984437ca3e5bbbe06daf95d4
status-firefox56: affected → fixed
(Assignee)

Updated

a year ago
Attachment #8911875 - Flags: approval-mozilla-beta?
Flags: qe-verify+
This is verified fixed in 56.0.2 and 57.b12.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.