Advanced settings UI change to centralize save state

[mbondyra]

Summary

Fixes #27405

Converts the contextual advanced settings menu to the shared one:
Before:

After:

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

• This was checked for cross-browser compatibility, including a check against IE11
• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for keyboard-only and screenreader accessibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately
• This includes a feature addition or change that requires a release note and was labeled appropriately

[cchaos]

@gchaps We'd love to get your help with some copy changes we're making to the Advanced Settings UI. The gist is that instead of having Save/Cancel buttons per setting, there will be a "dirty" state that will popup a single bottom bar with "Save all"/"Cancel all" buttons. There will also be indicators showing which settings have been changed but not saved.

Copy

1. Unsaved indicator (bottom left of bottom bar).
a. 2 Unsaved settings
b. When using search to narrow down settings, some settings may get hidden: 2 (1 hidden) Unsaved settings

2. Save/Cancel buttons since clicking will save/cancel ALL unsaved settings
a. Save changes
b. Cancel all changes

3. Disabled Save button tooltip
When one of the fields they've changed is invalid, we disable the Save button. Here is what the tooltip currently says:
You must fix all invalid settings before saving.

4. The very last setting (Telemetry) callout
This setting doesn't behave the same as the others (doesn't get added to the dirty state, but saves automatically) because of technical limitations. We want to add more description to the current callout to bring attention to the fact that it will be saved automatically. This is what we have so far:
This setting applies to all of Kibana and will be saved immediately.

5. Reload notification toast
Some settings require browser refreshes to take effect. We originally had a toast that also named the setting in the toast message:

But, since there could be more than one setting that requires a reload, we're thinking about making this message more generic like:
One or more of the saved settings requires a page reload to take effect.

[gchaps]

1. Unsaved indicator
I suggest singular for 1, and plural for more than one.

1 unsaved setting
2 unsaved settings
2 unsaved settings, 1 of which is hidden

2. Save/Cancel buttons
I think it's ok to eliminate the word "all" in both button labels.

Save changes
Cancel changes

3. Disabled Save button tooltip
Fix invalid settings before saving.

4. Telemetry
Changes to this setting apply to all of Kibana and are saved automatically.

5. Reload notification toast
One or more settings require you to reload the page to take effect.

[cchaos]

Thanks @gchaps ! My only comment is that 2 unsaved settings, 1 of which is hidden seems very wordy and will take up a lot of space. Any way to shorten it?

[gchaps]

How about:

2 unsaved settings, 1 hidden

[elasticmachine]

Pinging @elastic/kibana-app (Team:KibanaApp)

[myasonik]

@cchaos

My original thought about this form is that it now actually behaves as one giant form (with a submit button) instead small individual forms (with submit buttons). Therefore, I don't know why we'd need a "Setting is currently not saved." screen reader text when you don't indicate changed fields like this in normal forms.

In short, because it can be so difficult to navigate this page. (In detail: the page is long, has many tab stops, has many form elements, has many headings, isn't wrapped by an actual <form> and so doesn't have a form onsubmit event, even if we did wrap it in a <form> the BottomBar wouldn't be in it because of the portal, and the submit and cancel buttons aren't even in the same main landmark because of the portal as well.) The higher the complexity of the page, the more context we want to give users on what is going on.

I'm trying not to crowd this already giant form with more indicators of state. The thought behind this bar is that it does indicate there's something different about this setting over another setting that hasn't been changed. There's a stark contrast between existing and not existing indicates a difference. And the indicator shows as soon as they change something, hopefully showing causation of the indicator.

The bar and the contrast do show that something is different but what I'd also like to be able to solve is having to know which contrast means what. UI friendly to cognitive load (which might be compromised from permanent, temporary, or situational variables) will let you glean all the info you need just by looking at it without prior knowledge. So, especially when you combine this and the next point, looking at just a setting - I don't know if the green bar means this setting is "all good, don't worry about it" or, as it means in this case "you've changed something here".

The bottom bar exists in a portal that is fixed to the window so it doesn't scroll away with the content.

It doesn't scroll away and is visible on the page, but the two controls (the setting and the save) are now very far apart from each other. Screen zoom, which is different than your browser's page zoom, will usually clip off parts (often, most) of the page and only show you a small portion. Pinch-to-zoom on touch devices is an example of this (though, position:fixed will actually sometimes - but not always - be preserved in this case) but other examples of this are Microsoft's Magnifier, Mac's Zoom (Settings > Accessibility > Zoom), or 3rd-party products like ZoomText. (Links are to demo videos if you want to check them out.) Assistive tech usage numbers are really hard to come by and are often questionable when they do come up anyways, but from the community folks I follow on Twitter, this type of zooming tech is reportedly often more popular than screen readers because the learning curve is generally lower and sites have less ways of messing it up (where as screen readers can be entirely hampered by a bad site). So I'm hoping we can provide something that will hint to users that they still need to perform a second save action that they may not see right away.

Writing all of this up has lead me to an important question: What about auto-save? Some of the settings I know require a page refresh so many we can't do it with all but could most of them be saved immediately when changed?

[cchaos]

I'm going to restate @myasonik's question since it might have gotten lost in the rest of his comment:

What about auto-save? Some of the settings I know require a page refresh so many we can't do it with all but could most of them be saved immediately when changed?

[mbondyra]

Hi @cchaos @myasonik

We discussed the auto-save idea with @timroes and decided that it would cause more problems and requires additional design.

1. We describe advanced settings as 'for very advanced users only. (...) can break large portions of Kibana.' The save button is additional 'think again before you confirm' check. In the autosave scenario, there's no way for the user to come back to the previous value of the setting ('cancel' the change) and that's an important one. Adding another 'reset to previous value' button would solve a problem but it seems weird to have two reset buttons
2. For text inputs we would have to save on blur and that's not intuitive behaviour, so we would need to keep save button for these ones
3. Imagine the scenario: user modifies the value, saving is run, but network fails. What do we do in this case? 'Retry' button?

To improve accessibility on this page in the current state we want to:

1. Prevent from navigating away (alert on leaving the page), but this one requires help from app arch team so for now, it's impossible.
2. I added to the Title EuiScreenReaderOnly element for unsaved/invalid elements for text hint that element is unsaved.

Let me know your thoughts.

[myasonik]

Unfortunately this doesn't work because of the way that EUI renders forms at the moment (I opened a bug for it elastic/eui#2888).

I tried a few different things and the thing that I thought worked best was moving this from the renderTitle function to the renderField function.

For each field, add an aria-describedby="{uniqueID}". Then, in the render function, right after the {this.renderField(setting)} line, you can put all this stuff with the same uniqueId on the <p>.

Can talk through it more tomorrow!

[mbondyra]

@myasonik I corrected the code according to your comments. That, however adds aria-describedby to every element, even the ones that don't have relative <p> element. Is this ok?

[myasonik]

aria-describedby should always point to a real id that exists on the page.

The contents can be empty though.

Does that make sense?

[mbondyra]

@elasticmachine merge upstream

[mbondyra]

@elasticmachine merge upstream

[mbondyra]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 917e74e

History

• 💔 Build #28343 failed 6cd2486
• 💔 Build #28319 failed d1a3e8b
• 💔 Build #28315 failed 47b3d61
• 💔 Build #28244 failed 3fae234
• 💔 Build #27858 failed b657eb3

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[timroes]

Tested on Chrome linux, code LGTM so far. The remaining issues can imho be addressed in follow up PRs.

[myasonik]

Looks good for now 👍

In a future a11y PR I'd like to fix two things:

1. The visual indicator that we've talked about before
2. EuiSwitch currently has two labels (an aria-label and an aria-labelledby) which generally doesn't work (this may require work in EUI though in which case we can open up a ticket instead, I didn't dig deep enough to figure out where the issue is coming from)