Closed Bug 1159327 Opened 7 years ago Closed 7 years ago

[e10s] Accessibility blacklist client work

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
e10s m6+ ---
firefox40 --- fixed

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(1 file, 2 obsolete files)

We will likely roll e10s out in advance of having good a11y support. Due to this we will need a blacklist of clients we disable e10s for. This bug covers this work.
Blocks: 1159331
Assignee: nobody → jmathies
tracking-e10s: --- → m6+
Attached patch wip (obsolete) — Splinter Review
Hey David,

I need some help here deciding what we prompt to disable for. With this patch I prompt for every client we can detect UIAUTOMATION and of course UNKNOWN. Both of which we spoke about the other day.

On Mac and linux I'm not sure what to do. With this patch if a11y is found to be active we prompt to disable.

Does this seem right?
Attachment #8599483 - Flags: feedback?(dbolter)
> I need some help here deciding what we prompt to disable for. With this
> patch I prompt for every client we can detect UIAUTOMATION and of course
> UNKNOWN. Both of which we spoke about the other day.


to clarify - I prompt to disable e10s for every client we can detect except UIAUTOMATION and of course UNKNOWN.
I think this plan makes sense.
Attachment #8599483 - Flags: feedback?(dbolter) → feedback+
Trevor am I correct we need a bit more work to make things like focus events work etc before removing the opt-out for OTHER?
Flags: needinfo?(tbsaunde+mozbugs)
Comment on attachment 8599483 [details] [diff] [review]
wip

might as well kick this off.
Attachment #8599483 - Flags: review?(tbsaunde+mozbugs)
Flags: needinfo?(tbsaunde+mozbugs)
I tested this with JAWS and with Inspect.exe. For the former we prompted, for the latter we did not.
Comment on attachment 8599483 [details] [diff] [review]
wip

>+bool
>+IsAccBlacklistedForE10S()
>+{

given Compatability.h is exported this seems like a rather silly layer of  indirection.

>+#if defined(XP_WIN)
>+  return Compatibility::IsBlacklistedForE10S();
>+#else
>+  return true;
>+#endif

I think we could always return false on linux at this point.  It doesn't work nearly well enough to release, but you can certainly use chrome and see content some.

>+  static bool IsBlacklistedForE10S() {

{ on next line please

>+    // We currently blacklist everything except UNKNOWN and UIAUTOMATION
>+    return !!(sConsumers &
>+              (NVDA     |
>+               JAWS     |
>+               OLDJAWS  |
>+               WE       |
>+               DOLPHIN  |
>+               SEROTEK  |
>+               COBRA    |
>+               ZOOMTEXT |
>+               KAZAGURU |
>+               YOUDAO));

I'm tempted to not black list the last two, but I guess we can do that later.
Attachment #8599483 - Flags: review?(tbsaunde+mozbugs)
will update and repost, thanks.
(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> Comment on attachment 8599483 [details] [diff] [review]
> wip
> 
> >+bool
> >+IsAccBlacklistedForE10S()
> >+{
> 
> given Compatability.h is exported this seems like a rather silly layer of 
> indirection.

I was anticipating platform specific code for each platform. I guess that's not going to be the case so I'll take this out and just include windows Compatability.h in nsAppRunner if needed.


> >+#if defined(XP_WIN)
> >+  return Compatibility::IsBlacklistedForE10S();
> >+#else
> >+  return true;
> >+#endif
> 
> I think we could always return false on linux at this point.  It doesn't
> work nearly well enough to release, but you can certainly use chrome and see
> content some.

will do.

> >+               ZOOMTEXT |
> >+               KAZAGURU |
> >+               YOUDAO));
> 
> I'm tempted to not black list the last two, but I guess we can do that later.

Easy to add this now if you want.
(In reply to Jim Mathies [:jimm] from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > Comment on attachment 8599483 [details] [diff] [review]
> > wip
> > 
> > >+bool
> > >+IsAccBlacklistedForE10S()
> > >+{
> > 
> > given Compatability.h is exported this seems like a rather silly layer of 
> > indirection.
> 
> I was anticipating platform specific code for each platform. I guess that's
> not going to be the case so I'll take this out and just include windows
> Compatability.h in nsAppRunner if needed.

I think windows is the only place we can have a finer grained thing than on or off.

> > >+               ZOOMTEXT |
> > >+               KAZAGURU |
> > >+               YOUDAO));
> > 
> > I'm tempted to not black list the last two, but I guess we can do that later.
> 
> Easy to add this now if you want.

lets play it safe and see how more exposure for UIA and linux goes.
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #8599483 - Attachment is obsolete: true
Attached patch patch v.2Splinter Review
Attachment #8600001 - Attachment is obsolete: true
Attachment #8600002 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8600002 [details] [diff] [review]
patch v.2

>+  /**
>+   * Return true if we should disable e10s due to a detected
>+   * accessibility client.
>+   */
>+  static bool IsBlacklistedForE10S() {

you forgot to fix the formating here '{' belongs on next line.
Attachment #8600002 - Flags: review?(tbsaunde+mozbugs) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #14)
> Comment on attachment 8600002 [details] [diff] [review]
> patch v.2
> 
> >+  /**
> >+   * Return true if we should disable e10s due to a detected
> >+   * accessibility client.
> >+   */
> >+  static bool IsBlacklistedForE10S() {
> 
> you forgot to fix the formating here '{' belongs on next line.

fixed.
need an ifdef around that include. relanding here in a sec.
No longer blocks: 1159331
Duplicate of this bug: 1159331
https://hg.mozilla.org/mozilla-central/rev/796c77a8118b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Jim, any rumblings from nightly users? (How do we check/Can I help find out?)
Flags: needinfo?(jmathies)
(In reply to David Bolter [:davidb] from comment #22)
> Jim, any rumblings from nightly users? (How do we check/Can I help find out?)

haven't heard any complaints, no bugs filed either afaik. The disabled pref was reset as well so, things look good so far.
Flags: needinfo?(jmathies)
You need to log in before you can comment on or make changes to this bug.