Closed Bug 1387507 Opened 7 years ago Closed 7 years ago

Remove a11y e10s app runner disabling code in 57

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 + fixed

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: aes+)

Attachments

(2 files, 4 obsolete files)

There may be some related tests that we should unblock as e10s supports rides up beta and release?
http://searchfox.org/mozilla-central/search?q=RELEASE_OR_BETA&path=accessible
Flags: needinfo?(yzenevich)
(In reply to David Bolter [:davidb] from comment #1)
> There may be some related tests that we should unblock as e10s supports
> rides up beta and release?
> http://searchfox.org/mozilla-central/search?q=RELEASE_OR_BETA&path=accessible

I would double check with Eitan as he's the author, but I think you're right.
Flags: needinfo?(yzenevich) → needinfo?(eitan)
Yeah, those no longer need to be skipped.
Flags: needinfo?(eitan)
(In reply to Jim Mathies [:jimm] from comment #0)
> This code needs to go away in 57.
> 
> http://searchfox.org/mozilla-central/rev/
> 30a47c4339bd397b937abdb2305f99d3bb537ba6/toolkit/xre/nsAppRunner.cpp#5045

There are two additional patches from bug 1371051 that should be reverted as well.
Comment on attachment 8893908 [details] [diff] [review]
remove a11y e10s checks and prefs v.1

r? to felipe for browser glue changes
Attachment #8893908 - Flags: review?(felipc)
(In reply to Jim Mathies [:jimm] from comment #7)
> Comment on attachment 8893908 [details] [diff] [review]
> remove a11y e10s checks and prefs v.1
> 
> r? to felipe for browser glue changes

Actually I think you can review all of this felipe!
Attached patch enable tests (obsolete) — Splinter Review
Attachment #8894559 - Flags: review?(eitan)
Comment on attachment 8894559 [details] [diff] [review]
enable tests

Review of attachment 8894559 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #8894559 - Flags: review?(eitan) → review+
Attachment #8893908 - Flags: review?(felipc)
Depends on: 1385991
No longer blocks: post-57-api-changes
Blocks: 1385991
No longer depends on: 1385991
Ok, I think we're ready for this. We might have to hastily re-land this near the end of the cycle if we don't get testing. I'll keep track of the status on that.

Note I'm going to resurrect a little bit of this prompting code in a follow up bug related to older JAWS screen readers. To start though I'm going to clean this out fully.
Attachment #8893908 - Attachment is obsolete: true
Attachment #8900909 - Flags: review?(felipc)
Comment on attachment 8900909 [details] [diff] [review]
remove a11y e10s checks and prefs v.1

Review of attachment 8900909 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

Since there's a chance that this needs to be relanded at the end of beta, I think we should leave the strings unused and then file a follow-up to remove them when we're sure they won't be needed again.

::: dom/ipc/ContentParent.cpp
@@ -1317,1 @@
>        Unused << SendActivateA11y(::GetCurrentThreadId(),

uber nit: this line will end up with too much indent if it's not adjusted

@@ -2851,1 @@
>          Unused << SendActivateA11y(::GetCurrentThreadId(),

same
Attachment #8900909 - Flags: review?(felipc) → review+
Ah, and I just noticed, this needs to remove the e10s-64@2x.png file too and the references to it in the jar.mn files
(In reply to :Felipe Gomes (needinfo me!) from comment #12)
> Comment on attachment 8900909 [details] [diff] [review]
> remove a11y e10s checks and prefs v.1
> 
> Review of attachment 8900909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great!
> 
> Since there's a chance that this needs to be relanded at the end of beta, I
> think we should leave the strings unused and then file a follow-up to remove
> them when we're sure they won't be needed again.

Will do. I'll create new strings for the prompt in 1385991 as well so these strings remain unchanged.

> 
> ::: dom/ipc/ContentParent.cpp
> @@ -1317,1 @@
> >        Unused << SendActivateA11y(::GetCurrentThreadId(),
> 
> uber nit: this line will end up with too much indent if it's not adjusted
> 
> @@ -2851,1 @@
> >          Unused << SendActivateA11y(::GetCurrentThreadId(),
> 
> same

updated, thanks.
Attachment #8900909 - Attachment is obsolete: true
Attachment #8902749 - Flags: review+
both patches posted here.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab66385c2fb5
Remove a11y e10s checks and preferences. r=felipe
https://hg.mozilla.org/integration/mozilla-inbound/rev/e380765f9845
Enable disabled a11y tests. r=eitan
Keywords: checkin-needed
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccacb40ce6c7
Backed out changeset e380765f9845 
https://hg.mozilla.org/integration/mozilla-inbound/rev/658c8cac1523
Backed out changeset ab66385c2fb5 for failing browser-chrome's browser_all_files_referenced.js: unreferenced file e10s-64@2x.png. r=backout on a CLOSED TREE
argh, I guess that image will have to come out temporarily.
Flags: needinfo?(jmathies)
Recent try push, I'm seeing new a11y test failures. Need to sort this out as well before landing.
 
FAIL | accessible/tests/mochitest/events/test_selection.xul | Test timed out. [log…]
FAIL | accessible/tests/mochitest/treeupdate/test_menubutton.xul | Test timed out. [log…] 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0004894c99723bed99dd27e4fd86840cbb7f05e2
(In reply to Jim Mathies [:jimm] from comment #20)
> argh, I guess that image will have to come out temporarily.

Maybe you can land bug 1385991 at the same time to avoid that too
(In reply to Jim Mathies [:jimm] from comment #21)
> Recent try push, I'm seeing new a11y test failures. Need to sort this out as
> well before landing.
>  
> FAIL | accessible/tests/mochitest/events/test_selection.xul | Test timed
> out. [log…]
> FAIL | accessible/tests/mochitest/treeupdate/test_menubutton.xul | Test
> timed out. [log…] 
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0004894c99723bed99dd27e4fd86840cbb7f05e

Popped off my patch queue, pulled, and pushed just the changes here - clean a11y run. \0/

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1974cdfd8774c50c8e12a792a4429642fd890000
Jimm mentioned this bug fix will likely land in 57 as per the e10s/a11y plans
- fixed image issue associated with leftover icon

https://treeherder.mozilla.org/#/jobs?repo=try&revision=426152843e19e6afddbbc92c13d01ab4bfecb601
Attachment #8894559 - Attachment is obsolete: true
Attachment #8902749 - Attachment is obsolete: true
Attached patch enable testsSplinter Review
Pushed by jmathies@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a9bb3ff772
Remove a11y e10s checks and preferences. r=felipe
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b442d470556
Enable disabled a11y tests. r=eitan
You need to log in before you can comment on or make changes to this bug.