Preparation for High Contrast Mode, Core/SharedUX domains

[tsullivan]

Summary

Reviewers: Please test the code paths affected by this PR. See the "Risks" section below.

Part of work for enabling "high contrast mode" in Kibana. See #205411

Background:
Kibana will soon have a user profile setting to allow users to enable "high contrast mode." This setting will activate a flag with <EuiProvider> that causes EUI components to render with higher contrast visual elements. Consumer plugins and packages need to be updated selected places where <EuiProvider> is wrapped, to pass the UserProfileService service dependency from the CoreStart contract.

NOTE: EUI currently does not yet support the high-contrast mode flag, but support for that is expected to come in around 2 weeks. These first PRs are simply preparing the code by wiring up the UserProvideService.

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

• Unit or functional tests were updated or added to match the most common scenarios
• The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

• [medium/high] The implementor of this change did not manually test the affected code paths and relied on type-checking and functional tests to drive the changes. Code owners for this PR need to manually test the affected code paths.
• [medium] The UserProfileService dependency comes from the CoreStart contract. If acquiring the service causes synchronous code to become asynchronous, check for race conditions or errors in rendering React components. Code owners for this PR need to manually test the affected code paths.

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[elasticmachine]

Pinging @elastic/appex-sharedux (Team:SharedUX)

[afharo]

Core changes LGTM

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: e476cc2
• Storybooks Preview

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
@kbn/core-apps-browser-internal	22	23	+1
@kbn/react-kibana-context-root	4	5	+1
@kbn/react-kibana-context-theme	3	4	+1
kibanaReact	121	123	+2
kibanaUtils	417	418	+1
total			+6

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
home	152.2KB	152.2KB	-26.0B
share	3.6KB	3.6KB	-19.0B
total			-45.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
core	450.2KB	449.8KB	-391.0B
globalSearchBar	30.2KB	30.1KB	-30.0B
kbnUiSharedDeps-srcJs	3.5MB	3.5MB	+58.0B
kibanaReact	39.4KB	39.5KB	+62.0B
kibanaUtils	66.9KB	66.9KB	+56.0B
reporting	50.6KB	50.5KB	-22.0B
savedObjects	17.9KB	17.9KB	+36.0B
serverless	14.7KB	14.6KB	-52.0B
share	58.0KB	58.0KB	-28.0B
uiActions	24.2KB	24.3KB	+90.0B
total			-221.0B

Unknown metric groups

API count

id	before	after	diff
@kbn/core-apps-browser-internal	23	24	+1
@kbn/react-kibana-context-root	10	11	+1
@kbn/react-kibana-context-theme	13	15	+2
kibanaReact	153	155	+2
kibanaUtils	610	611	+1
total			+7

References to deprecated APIs

id	before	after	diff
aiAssistantManagementSelection	0	2	+2
observabilityAiAssistantManagement	0	2	+2
total			+4

History

• 💛 Build #257081 was flaky 1b4e99e
• 💚 Build #256693 succeeded cb47c51
• 💔 Build #256661 failed c79cd0b
• 💔 Build #256635 failed 844bf50
• 💔 Build #256265 failed ee5e0f9

[Dosant]

Not sure if there are other places in this PR to improve this, but I think this is the best way we can correctly pass these dependencies. the more often we do like this the better (less places to update in case we need something else from core) 👍

[tsullivan]

I fully agree, However there is a pattern where I think this wouldn't work very well, but I'm not sure and would like to get your thoughts. Say we have this in the code:

const showErrorToast = useCallback((errorTitle: string, error: Error) => {
  toasts.addDanger({
    title: errorTitle,
    text: toMountPoint(<Markdown>{error.message}</Markdown>, core);
  });
}, [toasts, core]);

Explanation: the toMountPoint function requires services from coreStart as a second parameter, because it creates a new rendering context. In useCallback, etc, the second parameter is an array of dependencies needed in the body of the callback. If a dependency changes, then the component re-renders. It could negatively impact performance if one of the dependencies is a large complex object. So the above code should change to:

const showErrorToast = useCallback((errorTitle: string, error: Error) => {
  toasts.addDanger({
    title: errorTitle,
    text: toMountPoint(<Markdown>{error.message}</Markdown>, { i18n, theme, analytics, userProfile });
  });
}, [toasts, i18n, theme, analytics, userProfile]);

Thoughts? @Dosant @sebelga @eokoneyo @clintandrewhall

[clintandrewhall]

This is an interesting example, but I think it's an edge case and is exempt from the pattern.

useCallback, useEffect and others always require forethought, and the second parameter is strongly typed. Still, we can code defensively within the contexts to handle this case.

We should destructure the incoming parameter in the KibanaRootContextProvider and perhaps also in the KibanaRenderContextProvider. This would effectively "discard" unused portions of core, before adding them to the context value, but still allow people to pass the entire construct if we were to need something else

KibanaRootContextProvider before:

    return (
      <KibanaEuiProvider {...props}>
        <i18n.Context>{children}</i18n.Context>
      </KibanaEuiProvider>
    );
  }

After:

    const {
      i18n,
      theme,
      userProfile,
      globalStyles
    } = props;

    return (
      <KibanaEuiProvider {...{ theme, userProfile, globalStyles }}>
        <i18n.Context>{children}</i18n.Context>
      </KibanaEuiProvider>
    );
  }

[tsullivan]

This is an interesting example, but I think it's an edge case and is exempt from the pattern.

Thanks @clintandrewhall. I was interested in thoughts/feedback about whether my understanding my understanding of useCallback performance is valid, and it sounds like you are saying that it is.

[clintandrewhall]

I am, but I'm also saying we should destructure the incoming parameters on the contexts.

Typescript doesn't prevent extraneous properties from being set to the context, so we should be sure to extract only those properties necessary.

[tsullivan]

Thanks @clintandrewhall. That is a solid observation and one that I can easily follow up with.

On the original subject of the performance concern with useCallback and using large objects, I want to include this message from @Dosant taken from a separate conversation:

broadly about re-rendering concern, hopefully as we pick up with react upgrades and in a year or two when this thing is stable, there will be a compiler that would optimize rendering so destructuring and new objects will no longer a performance concern https://react.dev/learn/react-compiler

[kibanamachine]

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12182853519

[kibanamachine]

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 202606

Questions ?

Please refer to the Backport tool documentation

[tsullivan]

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation