Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "feat(HighContrast): Expose the XAML HighContrastChanged event to JS" #1473

Closed
wants to merge 2 commits into from

Conversation

rozele
Copy link
Collaborator

@rozele rozele commented Oct 31, 2017

Reverts #1443

@rozele
Copy link
Collaborator Author

rozele commented Oct 31, 2017

@antonkh this is currently broken in master, please rebase and fix this work. cc @rigdern

@codecov-io
Copy link

codecov-io commented Oct 31, 2017

Codecov Report

Merging #1473 into master will decrease coverage by 0.61%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1473      +/-   ##
==========================================
- Coverage   31.47%   30.86%   -0.62%     
==========================================
  Files         266      267       +1     
  Lines       19296    19697     +401     
  Branches     1613     1716     +103     
==========================================
+ Hits         6073     6079       +6     
- Misses      13072    13466     +394     
- Partials      151      152       +1
Impacted Files Coverage Δ
...tNative.Shared/Modules/Network/NetworkingModule.cs 68.08% <0%> (-0.36%) ⬇️
ReactWindows/ReactNative.Shared/ReactRootView.cs 0% <0%> (ø) ⬆️
...ive.Shared/DevSupport/DisabledDevSupportManager.cs 0% <0%> (ø) ⬆️
...Windows/ReactNative.Shared/Bridge/ReactInstance.cs 0% <0%> (ø) ⬆️
...s/ReactNative.Shared/DevSupport/DevServerHelper.cs 0% <0%> (ø) ⬆️
...eactNative.Shared/Bridge/JavaScriptBundleLoader.cs 0% <0%> (ø) ⬆️
ReactWindows/ReactNative.Net46/ReactPage.cs 0% <0%> (ø) ⬆️
...tNative.Shared/ReactContextInitializedEventArgs.cs 0% <0%> (ø)
...ReactNative.Shared/DevSupport/DevSupportManager.cs 0.19% <0%> (+0.19%) ⬆️
...Windows/ReactNative.Shared/ReactInstanceManager.cs 1.01% <0%> (+1.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4858d42...74a1310. Read the comment docs.

@antonkh
Copy link
Contributor

antonkh commented Oct 31, 2017

What is broken? I'm testing it with S4L which is based on a slightly outdated (v0.46.0) fork of RNW.

@antonkh
Copy link
Contributor

antonkh commented Nov 1, 2017

I think I've found the problem you meant: in the Playground app I get an "Element not found" exception with a callstack referring to add_HighContrastChanged

@antonkh
Copy link
Contributor

antonkh commented Nov 1, 2017

Looks like in case of the playground app the event subscription happens on a wrong thread. I fixed it this way:

        public override void Initialize()
        {
            DispatcherHelpers.RunOnDispatcher(() =>
            {
                _accessibility.HighContrastChanged += OnHighContrastChanged;
            });
        }

It's not clear to me why in S4L it doesn't cause an exception.

@rozele
Copy link
Collaborator Author

rozele commented Nov 1, 2017

Ah, right, I think we updated so that Initialize and OnReactInstanceDispose run on the native module queue thread.

@antonkh
Copy link
Contributor

antonkh commented Nov 1, 2017

I've sent a PR to fix this: #1476

@rozele
Copy link
Collaborator Author

rozele commented Nov 1, 2017

Will close this and merge #1476

@rozele rozele closed this Nov 1, 2017
@rozele rozele deleted the revert-1443-master branch January 31, 2019 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants