JavaScript URLs in Tab component don't play nicely with strict CSP

[jimmydief]

The tab component uses an <a> element to wrap the tab contents but prevents default navigation behavior through the use of href="javascript:void(0);". This doesn't play nicely with CSP policies without the unsafe-inline directive which prevent the script from executing but also add a warning in the console.

design-system-react/components/tabs/private/tab.jsx

Line 148 in ecb0854

href="javascript:void(0);" // eslint-disable-line no-script-url

The SLDS blueprints for the tab component seem to suggest the Javascript URL approach as well so I'm not sure whether this issue really belongs there.

A potential solution would be to prevent the default behavior of click/keydown events where they're handled within the parent tabs component.

design-system-react/components/tabs/index.jsx

Lines 252 to 298 in ecb0854

	handleClick = (e) => {
	let node = e.target;
	/* eslint-disable no-cond-assign, fp/no-loops */
	do {
	if (this.isTabFromContainer(node)) {
	if (isTabDisabled(node)) {
	return;
	}
	let parentNode = node.parentNode; // eslint-disable-line prefer-destructuring
	if (parentNode.nodeName === 'LI') {
	node = node.parentNode;
	parentNode = node.parentNode; // eslint-disable-line prefer-destructuring
	}
	const index = [].slice.call(parentNode.children).indexOf(node);
	this.setSelected(index);
	return;
	}
	} while ((node = node.parentNode) !== null);
	/* eslint-enable no-cond-assign */
	};
	handleKeyDown = (event) => {
	if (this.isTabFromContainer(event.target)) {
	let index = this.getSelectedIndex();
	let preventDefault = false;
	if (event.keyCode === KEYS.LEFT || event.keyCode === KEYS.UP) {
	// Select next tab to the left
	index = this.getPrevTab(index);
	preventDefault = true;
	} else if (event.keyCode === KEYS.RIGHT || event.keyCode === KEYS.DOWN) {
	// Select next tab to the right
	index = this.getNextTab(index);
	preventDefault = true;
	}
	// Prevent any dumn scrollbars from moving around as we type.
	if (preventDefault) {
	EventUtil.trap(event);
	}
	this.setSelected(index, true);
	}
	};

Happy to contribute if there's an agreeable fix.

It looks like there are some more examples of this. #2463 seems likely related.

[garygong]

I wonder if we can just use a <button> instead of <a>, or maybe a <span> with a tabIndex.

[dcefram]

To add to this issue, latest versions of React is now throwing a warning due to the use of javascript:void(0). Actual warning:

console.error node_modules/react-dom/cjs/react-dom.development.js:88

Warning: A future version of React will block javascript: URLs as a security precaution. Use event handlers instead if you can. If you need to generate unsafe HTML try using dangerouslySetInnerHTML instead. React was passed "javascript:void(0);".

I also agree with using a button instead of an anchor tag, as is recommended here.

[stale]

This issue has been automatically marked as stale, because it has not had recent activity. It will be closed if no further activity occurs. Maintainers are responsible for tech debt and project health. This is most likely a new components or component feature request. Please submit a pull request for or request feedback on this feature. Thank you.

[jimmydief]

@garygong Can you reopen this one? I'd like to revive #2514.

[interactivellama]

@jimmydief I'm cool with not following the SLDS blueprint/HTML strictly. LWC components do not.

[jimmydief]

@interactivellama Mind taking a look at #2514? This seems like it would address this issue and unblock upgrading to React 16.9.0 and above for those who dislike deprecation warnings. There are some places where <a> => <button> makes a lot of sense (e.g. the tab component mentioned in this issue which is unlikely to navigate to another page) but others where the correct tag is more contextual (e.g. options in a combobox menu, some of which may navigate elsewhere while others autocomplete). Seems like it might be easier to address those on a case-by-case basis.

[jimmydief]

Fixed by #2829.