[BUG] Cannot open visualisations in new windows or tabs

[jgough]

Describe the bug
If I go to /app/dashboards then I can middle click or right click on a dashboard in the list and open in a new window/tab
If I go to /app/visualize then I cannot middle click or right click on a visualization in the list to open in a new window/tab

To Reproduce
Steps to reproduce the behavior:

1. Go to /app/visualize
2. Try to open a visualization in a new tab or window (either by middle clicking, or right clicking) and note that it is not possible

Expected behavior
I expected to be able to open visualizations in new tabs and windows, much like is possible for /app/dashboards.

Screenshots
Visualize:

Dashboards:

OpenSearch Version
1.3.4

Dashboards Version
1.3.4

Plugins
security-dashboards-plugin

Host/Environment (please complete the following information):

• Docker

[joshuarrrr]

@jgough Thanks for filing. As a fellow tab junkie, I agree this is annoying.

I can confirm that this behavior still exists in main by reproducing at https://playground.opensearch.org/app/visualize

I'm pretty sure the cause of the behavior is that the visualization list items are actually <button> elements, although they're styled like links. For a11y and semantic purposes, it would definitely be better to use actual <a> links instead, in which case the browser default behavior would work as expected.

One final note is that the the dashboards listing page is still actually rendered via Angular. When we update that list rendering (potentially in #1866), we need to make sure we don't actually introduce this same bug there.

[BSFishy]

With a quick inspect element, I can see that the issue is in fact that the element is a button on the visualize page instead of an a. Both pages seem to use a link component, so it should just be a matter of telling the component to use an a instead of a button.

I was trying to find the exact location of the issue just by browsing on Github, but I got a little lost. From what I did see, though, it looks like it may be something that needs to be changed in OUI, but I'll look more into it on Monday.

[seraphjiang]

Thanks @BSFishy for the research

@joshuarrrr @bandinib-amzn would you help @BSFishy to locate the issue.

cc: @AMoo-Miki in case the fix need to change in Oui.

[BSFishy]

I've found the cause of the issue by cross-referencing the code for the dashboard page. It seems that the link for the visualize page doesn't actually have an href attribute:

OpenSearch-Dashboards/src/plugins/visualize/public/application/utils/get_table_columns.tsx

Lines 104 to 115 in 77af7f9

	<EuiLink
	onClick={() => {
	if (editApp) {
	application.navigateToApp(editApp, { path: editUrl });
	} else if (editUrl) {
	history.push(editUrl);
	}
	}}
	data-test-subj={`visListingTitleLink-${title.split(' ').join('-')}`}
	>
	{field}
	</EuiLink>

Compared to the dashboard page's:

OpenSearch-Dashboards/src/plugins/dashboard/public/application/listing/dashboard_listing.js

Lines 172 to 177 in 77af7f9

	<EuiLink
	href={this.props.getViewUrl(record)}
	data-test-subj={`dashboardListingTitleLink-${record.title.split(' ').join('-')}`}
	>
	{field}
	</EuiLink>

I'm not super familiar with OUI, but maybe we could just add the href as editUrl and then in the onClick handler cancel default?

To get it to open in a new tab, we can't just cancel default, I don't think. I don't think it would be wise to try to figure out if the link should be opened in a new tab in the event handler, because that info isn't passed to the event. Not sure what application.navigateToApp is meant to do. Can it be omitted and we switch entirely to use href instead of onClick?

[seraphjiang]

for Eui, we coud refer to https://elastic.github.io/eui/#/navigation/link

@AMoo-Miki Does Oui has equivalent doc site.

[AMoo-Miki]

@AMoo-Miki Does Oui has equivalent doc site.

There is one coming up soon.

[BSFishy]

I had a quick conversation with @joshuarrrr, and we discussed if the onClick behavior could be replaced with a regular link. It basically came down to whether or not the logic in application.navigateToApp is trivial. Here is the relevant code:

OpenSearch-Dashboards/src/core/public/application/application_service.tsx

Lines 252 to 270 in a19a741

	const navigateToApp: InternalApplicationStart['navigateToApp'] = async (
	appId,
	{ path, state, replace = false }: NavigateToAppOptions = {}
	) => {
	const currentAppId = this.currentAppId$.value;
	const navigatingToSameApp = currentAppId === appId;
	const shouldNavigate = navigatingToSameApp ? true : await this.shouldNavigate(overlays);
	if (shouldNavigate) {
	if (path === undefined) {
	path = applications$.value.get(appId)?.defaultPath;
	}
	if (!navigatingToSameApp) {
	this.appInternalStates.delete(this.currentAppId$.value!);
	}
	this.navigate!(getAppUrl(availableMounters, appId, path), state, replace);
	this.currentAppId$.next(appId);
	}
	};

As far as I'm aware, the only thing that the function is doing outside of changing the page is setting internal state, which should be done when the page is loaded through clicking the link. The application variable also has a method, getUrlForApp, to get the URL to redirect to, so I'm going to test if setting the href to that output and removing the onClick works as intended.

[joshuarrrr]

Resolved via #2395