Dialogs: provide a simpler internal API that avoids having to use button index magic

[rehmsen]

When providing a cancelId, VSCode on Linux/Mac reverses the button order and moves the cancel button in the second right most position:

vscode/src/vs/base/browser/ui/dialog/dialog.ts

Line 489 in f078e6d

// macOS/linux: reverse button order if `cancelId` is defined

This means that when you have e.g. three buttons, Cancel will be between the other two.

I could not find more documentation on it, so I am not sure what exactly the intent is. According this website Windows prefers Cancel to be the right most button, and MacOS prefers an action button to be right most, and Cancel to the left of that. What is not completely clear from this guideline is what should happen if there are three buttons (say primary action, secondary action, cancel). I would have expected cancel to be either on the far left or far right, but not between the primary and secondary action. The first example for VSCode's webview button guidelines also seems to suggest that Cancel is not between "Publish" and "Create" but, in this case, to the left.

More generally, reordering (only) when cancelId is provided is a surprising side effect, and it should probably be documented that input order is expected to be Windows-style (Cancel last).

I would be happy to send a PR with some documentation around this, and would also love to send a change to avoid having the cancel button in the middle between multiple action buttons, but I first wanted to better understand what the intention in change 3074fd5 by @lramos15 was.

Thanks for your time!

[sbatten]

@rehmsen Thanks for the detailed issue. Currently the dialog API for extensions is not so explicit so that would explain the lack of clarity. This came up recently and this is good feedback to apply in those changes.

However, its not clear to me if you are referring to the extensions API or the service within the product itself. Since you refer to the service.

[rehmsen]

Thanks for the response Steven!

I am from Google and we are using mainly IDialogService.show. I found these doc strings:

vscode/src/vs/platform/dialogs/common/dialogs.ts

Lines 273 to 285 in f078e6d

	/**
	* Present a modal dialog to the user.
	*
	* @param severity the severity of the message
	* @param message the message to show
	* @param buttons the buttons to show. By convention, the first button should be the
	* primary action and the last button the "Cancel" action.
	*
	* @returns A promise with the selected choice index. If the user refused to choose,
	* then a promise with index of `cancelId` option is returned. If there is no such
	* option then promise with index `0` is returned.
	*/
	show(severity: Severity, message: string, buttons?: string[], options?: IDialogOptions): Promise<IShowResult>;

They do mention that by convention the cancel button should be passed in last. They do not mention the side-effect of cancelId to reverse the buttons on Mac/Linux, and to move the button with the cancelId to the second last position. I could send a PR documenting that, but I think it would be good to first understand

1. Why we only reverse the button ordering when a cancelId is provided - this sounds like the right thing to do independent of the cancelId, to comply to the UX expectations on the respective platforms.
2. Why we move the cancel button into the second last position. I find it a bid odd to have [secondary] [cancel] [primary], with cancel in the middle. But given there is code that does this, maybe this is intended, and then it would help to understand why. This is the part that also seems to not be aligned with the screenshots from the webview button guidelines.

I am happy to help by sending the PRs, just want to make sure we are aligned on what the behavior should be and why.

[rehmsen]

Hi Steven,

as it is often easier to discuss a change with a proposal, I have send a PR for a first step in the direction of making this behavior less unexpected: Adding a new option which controls the reordering behavior instead of making that a side-effect of cancelId. As I am a new contributor to this repo, it seems I need a maintainers approval to run checks. I did not find unit tests for this file, so I did not update any, let me know if you would like me to add some (which would probably become a larger change as my change is in a private function that cannot be tested in full isolation).

If am planning to send a second PR for fixing

fileDialogService.ts:235 uses cancelId=-1 to indicate there is no cancel button - which has the probably undesired side-effect of moving the last button ("Open files" or "Learn more") to the second last position because of how slice works with negative indices.

If I get these two PRs merged, I would like to work with you / your UX person to reconsider moving the cancel button second last - but let's first see if we can agree that this more explicit reordering is an improvement.

Thank you for your time, and please let me know if there is anything I can improve to simplify the merging.

Cheers
Ole

[rehmsen]

#171595 is a PR to fix the apparently undesired button reordering in the file dialog.

[bpasero]

I think the original motivation to move Cancel between 2 primary actions was to somewhat emulate macOS when there are 2 buttons and 1 cancel button:

Electron does not provide us with an API to do this (having this gap between the buttons) and besides, with more recent macOS versions buttons are also ordered from top to bottom (interestingly not in this case of TextEdit).

Interestingly enough, Xcode is putting "Cancel" all the way to the left:

Sometimes, but sometimes not:

macOS HIG indicate:

Place buttons where people expect. In general, place the button people are most likely to choose on the trailing side in a row of buttons or at the top in a stack of buttons. Always place the default button on the trailing side of a row or at the top of a stack. Cancel buttons are typically on the leading side of a row or at the bottom of a stack.

From: https://developer.apple.com/design/human-interface-guidelines/components/presentation/alerts/

Before making any changes here on how to reorder we really need 100% understanding what the right thing to do is, based on the OS.

Worth noting that a change of button order will have an impact for those that have built muscle memory where to click.

[rehmsen]

Thanks for providing context Benjamin. From what you write, it sounds like VSCode is currently not in alignment with MacOS standards at least (as they suggest to typically place "Cancel" at the leading side). However, the two PRs I send so far do not touch the position of the cancel button, only make it more explicit when to do the reordering. So these should be fine (I will respond to your comment on one of them there).

[bpasero]

Just to clarify: when I look at a very typical dialog, "Cancel" appears at the end:

So I am not sure in what case we are inconsistent with the HIG.

[rehmsen]

For example when you open a file from an untrusted workspace, you get

Notice how cancel is in the middle, while the HIG you quote says:

Cancel buttons are typically on the leading side of a row or at the bottom of a stack.

[bpasero]

Yes, sorry, I was not carefully looking. There seems to be a major flaw in how native dialogs vs. custom dialogs are handled with regards to ordering when cancelId is set. To summarise:

Native Dialogs:

the cancel button should always be to the left of the primary action
if we see more than 2 buttons, move the cancel one to the left of the primary

This seems wrong on macOS at least (not sure about Linux) given the HIG we found. So I would suggest to change this according to HIG (also on Linux if we can confirm). I would think that:

• if no cancelId is provided, we assume the caller has the order set that makes sense
• if cancelId is provided, we should ensure its moved to the other edge away from primary button

vscode/src/vs/workbench/electron-sandbox/parts/dialogs/dialogHandler.ts

Lines 133 to 143 in 3fd6567

	if (!isWindows && buttons.length > 2 && cancelId !== 1) {
	const cancelButton = buttons[cancelId];
	buttons.splice(cancelId, 1);
	buttons.splice(1, 0, cancelButton);
	const cancelButtonIndex = buttonIndexMap[cancelId];
	buttonIndexMap.splice(cancelId, 1);
	buttonIndexMap.splice(1, 0, cancelButtonIndex);
	cancelId = 1;
	}

Custom Dialogs:

I do not fully understand the code, it seems wrong:

• why are we not reversing the buttons on Linux and macOS?
• why do we only reverse if cancelId is provided

vscode/src/vs/base/browser/ui/dialog/dialog.ts

Lines 490 to 497 in 3fd6567

	// macOS/linux: reverse button order if `cancelId` is defined
	if (isMacintosh || isLinux) {
	if (cancelId !== undefined && cancelId < buttons.length) {
	const cancelButton = buttonMap.splice(cancelId, 1)[0];
	buttonMap.reverse();
	buttonMap.splice(buttonMap.length - 1, 0, cancelButton);
	}
	}

I think if we want to fix this, we should do it consistently for native and custom dialogs so that they appear the same 👍