Closed
Bug 1182097
Opened 10 years ago
Closed 9 years ago
Disable e10s for a11y users in FF41
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: davidb, Assigned: tbsaunde)
References
Details
Attachments
(2 files, 1 obsolete file)
1.54 KB,
patch
|
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
878 bytes,
patch
|
tbsaunde
:
review+
davidb
:
feedback+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
We're not ready.
Updated•10 years ago
|
tracking-e10s:
--- → ?
Assignee | ||
Updated•10 years ago
|
QA Contact: tbsaunde+mozbugs
Comment 1•10 years ago
|
||
We're going to leave this in triage and work on these crasher bugs, we'll revisit this next tuesday.
Assignee | ||
Comment 2•10 years ago
|
||
IPC accessibility is not nearly ready for use and causes problems. Its not
worth backporting all the patches that would be needed to fix the usability so
there's no real point in fixing the crashes it causes. Instead just disable
the IPC code.
Attachment #8632228 -
Flags: review?(dbolter)
Reporter | ||
Comment 3•10 years ago
|
||
I'll see how triage goes today.
Updated•10 years ago
|
Comment 4•10 years ago
|
||
If we're going to do this, shouldn't we bring back the disable prompt instead? With this patch, what happens if the user turns e10s on and they are using a11y?
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #4)
> If we're going to do this, shouldn't we bring back the disable prompt
> instead?
Yes I think so.
> With this patch, what happens if the user turns e10s on and they
> are using a11y?
Accessible content would be broken.
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to David Bolter [:davidb] from comment #5)
> (In reply to Jim Mathies [:jimm] from comment #4)
> > If we're going to do this, shouldn't we bring back the disable prompt
> > instead?
>
> Yes I think so.
In principal sure, I'm not sure its worth the bother though, its only aurora after all.
Reporter | ||
Comment 7•10 years ago
|
||
Trevor, Jim, to show the correct prompt is it as simple as having IsBlacklistedForE10S always return true?
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(jmathies)
Reporter | ||
Comment 8•10 years ago
|
||
Comment on attachment 8632228 [details] [diff] [review]
disable ipc accessibility on the firefox 41 branch
Review of attachment 8632228 [details] [diff] [review]:
-----------------------------------------------------------------
I think we need to decide on the prompt before landing this.
Attachment #8632228 -
Flags: review?(dbolter) → review+
Comment 9•10 years ago
|
||
(In reply to David Bolter [:davidb] from comment #8)
> Comment on attachment 8632228 [details] [diff] [review]
> disable ipc accessibility on the firefox 41 branch
>
> Review of attachment 8632228 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I think we need to decide on the prompt before landing this.
Yeah that looks like it would work.
Flags: needinfo?(jmathies)
Updated•10 years ago
|
Assignee: nobody → tbsaunde+mozbugs
Assignee | ||
Comment 10•10 years ago
|
||
IPC accessibility is not nearly ready for use and causes problems. Its not
worth backporting all the patches that would be needed to fix the usability so
there's no real point in fixing the crashes it causes. Instead just disable
the IPC code.
Attachment #8632228 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8636048 [details] [diff] [review]
disable ipc accessibility on the firefox 41 branch
Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]:
[String/UUID change made/needed]:
Attachment #8636048 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 12•10 years ago
|
||
forgot to fill out form first time.
> Approval Request Comment
> [Feature/regressing bug #]:
> [User impact if declined]:a11y crashes when e10s is enabled
> [Describe test coverage new/current, TreeHerder]:
> [Risks and why]: low just disables code that only runs with e10s on
> [String/UUID change made/needed]: none
Comment 13•10 years ago
|
||
Trevor, while reviewing the aurora-uplift request, I noticed that the said patch has not been r+'d? Or has it?
Flags: needinfo?(tbsaunde+mozbugs)
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #13)
> Trevor, while reviewing the aurora-uplift request, I noticed that the said
> patch has not been r+'d? Or has it?
it was in the previous attachment.
Flags: needinfo?(tbsaunde+mozbugs)
Comment 15•10 years ago
|
||
Comment on attachment 8636048 [details] [diff] [review]
disable ipc accessibility on the firefox 41 branch
Approving for uplift to Aurora. Seems to be a safe patch.
Attachment #8636048 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
status-firefox41:
--- → affected
status-firefox42:
--- → wontfix
Comment 16•10 years ago
|
||
Flags: in-testsuite-
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 17•10 years ago
|
||
With the forcing of accessibilityIsBlacklistedForE10S to true, this means that every user on Aurora will get a popup warning about e10s not being compatible with e10s, and will have e10s disabled as the default operation of that popup.
Is that part of the change really needed? That flag is used as "a11y is blocking e10s, so deactivate e10s", rather than "..., so deactivate a11y"
Updated•10 years ago
|
Flags: needinfo?(tbsaunde+mozbugs)
Reporter | ||
Comment 18•10 years ago
|
||
We should probably check GetAccService() before returning true.
Comment 19•10 years ago
|
||
Doesn't appear to be hurting our E10S_AUTOSTART telemetry numbers, fwiw. Definitely needs a fix though, this isn't an excuse to keep this.
Reporter | ||
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 20•10 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #19)
> Doesn't appear to be hurting our E10S_AUTOSTART telemetry numbers, fwiw.
> Definitely needs a fix though, this isn't an excuse to keep this.
This is because there hasn't been an Aurora update to users with this patch yet, aiui.
Comment 21•10 years ago
|
||
I think this is the simplest change from what dbolter suggested
Attachment #8641249 -
Flags: review?(dbolter)
Reporter | ||
Comment 22•10 years ago
|
||
Comment on attachment 8641249 [details] [diff] [review]
fix accessibilityIsBlacklistedForE10S flag
Review of attachment 8641249 [details] [diff] [review]:
-----------------------------------------------------------------
Yep thanks, I want Trevor to review.
Attachment #8641249 -
Flags: review?(tbsaunde+mozbugs)
Attachment #8641249 -
Flags: review?(dbolter)
Attachment #8641249 -
Flags: feedback+
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8641249 [details] [diff] [review]
fix accessibilityIsBlacklistedForE10S flag
yeah, I'm an idiot and read the comments here and didn't think :(
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8641249 -
Flags: review?(tbsaunde+mozbugs) → review+
Comment 24•10 years ago
|
||
Ah don't worry. Trevor, quick question: this just landed on Aurora, right? (i.e. there was no m-c version in some other bug for example)
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to :Felipe Gomes from comment #24)
> Ah don't worry. Trevor, quick question: this just landed on Aurora, right?
> (i.e. there was no m-c version in some other bug for example)
correct, it never landed on m-c
Comment 26•10 years ago
|
||
Comment on attachment 8641249 [details] [diff] [review]
fix accessibilityIsBlacklistedForE10S flag
Approval Request Comment
[Feature/regressing bug #]: follow-up to the previous patch on this bug
[User impact if declined]: users on Aurora will see a warning saying that e10s will be disabled because of a11y
[Describe test coverage new/current, TreeHerder]: I don't think there are tests for this
[Risks and why]: this code is only used to warn if e10s should be force-disabled when a11y is enabled. Given that a11y is being disabled for e10s, it should not have any big impact.
[String/UUID change made/needed]: -
Attachment #8641249 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Comment 27•10 years ago
|
||
Felipe, could you please confirm (by manually testing on a local build) that after applying the second patch that e10s is disabled for accessibility users? And that e10s prompts for a opt-in at startup like before.
In your aurora uplift request you did not mention whether you tested it manually. I would like to get a confirmation from you so we know that we have the right fix this time. Thanks!
Flags: needinfo?(felipc)
Comment 28•10 years ago
|
||
I just tested manually by doing the following:
On Mac, I enabled VoiceOver and started an Aurora build with e10s previously enabled. The popup saying that a11y is incompatible with e10s showed up as expected and e10s was disabled after a restart.
This patch doesn't touch anything around the opt-in e10s prompt, but I double checked and it is appearing correctly.
Flags: needinfo?(felipc)
Comment 29•10 years ago
|
||
Comment on attachment 8641249 [details] [diff] [review]
fix accessibilityIsBlacklistedForE10S flag
Approved based on confirmation from Felipe that this patch does what is intended.
Attachment #8641249 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 30•9 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 10 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•