Closed
Bug 1159327
Opened 9 years ago
Closed 9 years ago
[e10s] Accessibility blacklist client work
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: jimm, Assigned: jimm)
References
Details
Attachments
(1 file, 2 obsolete files)
6.36 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jmathies
tracking-e10s:
--- → m6+
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
> 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.
Comment 3•9 years ago
|
||
I think this plan makes sense.
Updated•9 years ago
|
Attachment #8599483 -
Flags: feedback?(dbolter) → feedback+
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8599483 [details] [diff] [review] wip might as well kick this off.
Attachment #8599483 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(tbsaunde+mozbugs)
Assignee | ||
Comment 6•9 years ago
|
||
I tested this with JAWS and with Inspect.exe. For the former we prompted, for the latter we did not.
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 8•9 years ago
|
||
will update and repost, thanks.
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Comment 10•9 years ago
|
||
(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.
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8599483 -
Attachment is obsolete: true
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8600001 -
Attachment is obsolete: true
Attachment #8600002 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 13•9 years ago
|
||
try builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c18a130dc152
Comment 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
(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.
Comment 17•9 years ago
|
||
Backed out for bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/0d22cead404e https://treeherder.mozilla.org/logviewer.html#?job_id=9456333&repo=mozilla-inbound
Assignee | ||
Comment 18•9 years ago
|
||
need an ifdef around that include. relanding here in a sec.
Comment 21•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/796c77a8118b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 22•9 years ago
|
||
Jim, any rumblings from nightly users? (How do we check/Can I help find out?)
Flags: needinfo?(jmathies)
Assignee | ||
Comment 23•9 years ago
|
||
(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.
Description
•