Title Bar shifts when switching between saved and save

[n2erjo00]

What?

Removed Saved string from components button text/aria-label

Why?

Fixes #52295

How?

Testing Instructions

Check the original issue how to replicate the problem but observe now how button width remains the same and layout of header does not shift.

Testing Instructions for Keyboard

Screenshots or screencast

[jameskoster]

Thanks for the pull request. Unfortunately I don't think this solution will cover other languages where "save" and "saved" can be varying lengths. Did you have any other ideas?

[Mamaduka]

@jameskoster, maybe we should drop different label text; let the button's disabled state and snackbar communicate the saved state?

[jameskoster]

Yes I considered that, but I seem to recall the label change being an a11y requirement. Perhaps @alexstine can confirm?

Alex: would it be okay for the save button label to say "Save" (rather than "Saved") when the button is disabled?

[n2erjo00]

Agreed if we need to support every language version then button text cannot change. My (educated) guess would be that text changes because of accessibility. I think there was specific aria-* attribute for that (aria-busy, aria-loading something like that)

I think I have time time to do that upcoming weekend

Edit: Then again changing aria-attributes or text might/is bit over-engineering. Toggling disabled attribute should be enough
☝️ Personal opinion

[alexstine]

Preventing the text change is okay but you need to handle this with ARIA. Remember the basics, if sighted people see a loading state, it should be communicated to screen readers.

The disabled attribute is not something that should be used here due to the focus issues. There is an experimental prop on the Button component that allows us to deal with this better.

[Mamaduka]

Thank you, @alexstine!

The "Save" button already communicates its state using the ARIA attributes, just like the "Publish" button in the post editor. So I think we should be safe just to change the text.

@n2erjo00, do you want to work on this?

[n2erjo00]

Yes I can 😁
I'll do this during next weekend

[n2erjo00]

What you think about the post editors Update button. It also has text change happening but it does not make header layout shift. Should we in the name of consistency use static text there too?

Screen.Recording.2023-07-16.at.17.33.15.mov

[jameskoster]

Possibly yes, but let's look into that one separately.

For this PR, your implementation will also affect the button states when previewing a theme. It would probably be better to update getLabel() instead of forcing the static value for all use cases.

[Mamaduka]

I think we can just drop the disabled label string from the getLabel helper. You don't need to refactor it into multiple helpers.

The label and button text should be the same.

Why

The label also renders the aria-label attribute, and it should match the text content of the button.

Usually, we would omit the label when a button has the text content, but it's needed here for the tooltip and consistency with other buttons.

Example

const getLabel = () => {
	if ( isPreviewingTheme() ) {
		if ( isSaving ) {
			return __( 'Activating' );
		} else if ( isDirty ) {
			return __( 'Activate & Save' );
		}
		return __( 'Activate' );
	}

	if ( isSaving ) {
		return __( 'Saving' );
	} else if ( defaultLabel ) {
		return defaultLabel;
	}
	return __( 'Save' );
};

[n2erjo00]

Removed the "Saved" string from the component.

Note: header layout shifts when user hits "Save" since the text changes.

[carolinan]

Hi @n2erjo00 are you available to update the pull request with the latest changes from trunk, and make sure that the final playwright tests run (and pass).

[n2erjo00]

@carolinan Yeah I am here. I'll do updates during this week

[n2erjo00]

@carolinan Updated to latest trunk. I don't really remember where this was left before it went stale but looking forward of community input and to see where it goes 😄