Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow clicks to bubble and respect defaultPrevented #32

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thomas-darling
Copy link

@thomas-darling thomas-darling commented Dec 4, 2017

The DefaultLinkHandler attaches a click listener to the document element, which ensures a click on any anchor element activates the router.
Unfortunately, it listens for this event in the capture phase, which means no other handlers will ever get a chance to handle the event.
This makes it impossible to intercept the click and execute code before the navigation occurs, e.g. to cancel the navigation or track the click.

The DefaultLinkHandler also does not consider whether preventDefault() has been called on the event. This makes it impossible to cancel the navigation.

To resolve this, and to be consistent with how links natively work, this pull request:

  • Changes DefaultLinkHandler to listen for clicks in bubble phase instead of capture phase.
  • Changes DefaultLinkHandler to not navigate if preventDefault() was called on the click event.

@EisenbergEffect
Copy link
Contributor

@thomas-darling This makes good sense. Can we think of any scenarios where this would break existing code?

@thomas-darling
Copy link
Author

thomas-darling commented Dec 19, 2017

Sorry for the delay - we've actually had this fix in production for more than a year already, with no issues.
However, after a bit of testing, it actually looks like it could cause some breakage in code that wasn't written to take it into account - our code was, but other projects won't be.

Basically, the current behavior when clicking a link is this:

  1. The event is captured by DefaultLinkHandler
  2. The app navigates to the new route.
  3. The handler specified in a click.trigger on the <a> element is called.
  4. The handler specified in a click.delegate on the <a> element is called.

After this change, the behavior would be:

  1. The handler specified in a click.trigger on the <a> element is called.
  2. The handler specified in a click.delegate on the <a> element is called.
  3. The event bubbles up to DefaultLinkHandler
  4. The app navigates to the new route, unless event.defaultPrevented == true.

Now, the problem is, that unless you explicitly return true in the handlers attached with trigger and delegate, the framework automatically calls event.preventDefault(), which then prevents the navigation.
So in other words, this fix would actually break any links that also have a click handler attached, unless those handlers are changed to return true.

I'm not sure how many projects would be impacted by this, as anyone relying on the current behavior would have their code execute in a somewhat strange order - but who knows what people out there may have come up with. Therefore, I guess this does technically qualify as a breaking change :-(

A reasonable question here might be why trigger and delegate cancel the event automatically?
I can see how it might be handy for preventing e.g. arrow keys or space from scrolling the page, but it is non-standard and slightly unexpected behavior. I'm currently undecided as to how I feel about this...

Possible ways forward:

Option 1.

As it stands now, people would need to add a return true statement to event handlers attached to <a> elements with an href attribute - this probably won't cause any issues, although it theoretically could, if they also have other handlers that relied on the event being cancelled.

Assuming people don't do anything too crazy or custom though, it should be relatively easy to find any potential issues, by searching all views in an application using a regex such as:
<a .*href.*click.(trigger|delegate)|<a .*click.(trigger|delegate).*href

Option 2.

Alternatively, we could make this work, without breaking things, by having trigger and delegate tag the event with e.g. event.defaultPreventedByAurelia = true. That way, DefaultLinkHandler would know that it should proceed with the navigation, even if event.defaultPrevented == true.

We then run into the issue that a handler that does not return a value might still explicitly call event.preventDefault() - this would be incorrectly interpreted as an automatic cancellation.
To work around that, we probably have to also override the event.preventDefault() function, such that if it is called by anything other than Aurelia (i.e. without some magic argument), it sets event.defaultPreventedByAurelia = false.

Not sure how I feel about this, but it's the only option I can think of right now, that won't break things.
And for what it's worth, we actually already do very similar event tagging in our own custom control library, as it's really the only way of making certain interactions between controls work...

Thoughts?

@thomas-darling
Copy link
Author

After thinking more about this, my personal opinion is that we should go ahead an merge this, and then warn people that this is a potentially breaking change. It should be easy enough to fix by adding return true where appropriate, and I think the behavior introduced by this pull request is how this should work.

Basically, if you have click.trigger or click.delegate on a link that also has an href attribute, chances are that you want the app to control what happens when that link is clicked - the href is really just there for SEO reasons, so the link can be followed by crawlers, and to allow users to open the link in a new tab.

I'd argue that anyone who relies on the current behavior, where the click handlers execute after the app has already navigated, would be doing state management in a very strange way...

@EisenbergEffect
Copy link
Contributor

@davismj Can you look into this? It's been sitting here a while...

@davismj davismj self-assigned this Aug 1, 2018
@davismj
Copy link
Member

davismj commented Aug 1, 2018

@thomas-darling What is the use case for this change? Why would I click on a link and want it to do anything other than navigate to the link?

@thomas-darling
Copy link
Author

The primary use case we have encountered has to do with tracking clicks.
For example, we have a tracking infrastrukture which essentially allows us to write <a click.track=“my-link” .... This automatically attaches a click handler to that link, and then tracks any clicks on it using the name “my-link” - because when it comes to understanding usage patterns, it is important which link was clicked, not just which page a user navigated to.

Another use case would be a link which needs an href so it can be opened in another tab, but for performance or animation reasons, we may want to do something different when it is just clicked normally.

Makes sense? 🙂

@davismj
Copy link
Member

davismj commented Aug 1, 2018

Yes, thank you. To be sure, have you tried <a click.capture="track("my-link")">?

@thomas-darling
Copy link
Author

Yeah, the problem is, the Aurelia link handling uses capture too, and it attaches at the document level, before my code gets a chance to run - and is therefore always executed first.

@davismj
Copy link
Member

davismj commented Aug 1, 2018

Thanks for the additional information! That gives context for this PR.

@bigopon
Copy link
Member

bigopon commented Aug 1, 2018

@thomas-darling we know that because DefaultLinkHandler attaches first, and in capturing phase, so we can work around by telling it to remove and re-listen by calling deactivate and then activate. We would need to get the history instance and poke into .linkHandler to do that :

// main.js
export async function configure(aurelia) {
  
  // ... standard stuff
  await aurelia.start();

  // --------------------------------------------------
  // assume we all use this default implementation
  const linkHandler = aurelia.container.get(History).linkHandler; 
  linkHandler.deactivate();

  // then we do:
  document.addEventListener('click', (e) => {
    const trackedClickEvent = new CustomEvent('tracked-click', { bubbles: true });
    document.dispatchEvent(trackedClickEvent);
    
    // This is for better control, some track click may stop navigation, do stuff and then
    // resume navigation, so we use this flag to control it
    if (trackedClickEvent.defaultPrevented) {
      e.stopPropagation();
      // This is to dismiss any listener attached after this
      e.stopImmediatePropagation();
    }
  });
  // reactivate it
  linkHandler.activate();

  // --------------------------------------------------
  // ... set application root to start, standard stuff
}

Then in template, .track syntax basically only need to listen to tracked-click event on document, or i guess we can even do:

<a tracked-click.delegate="track($event)"></a>

Note: I haven't tried this yet

@davismj
Copy link
Member

davismj commented Aug 1, 2018

@bigopon seems like a pretty difficult workaround, don't you think?

@bigopon
Copy link
Member

bigopon commented Aug 1, 2018

Its pretty difficult to work around but the workaround is not difficult 😁

@thomas-darling
Copy link
Author

That workaround is way too complicated, for something that really should just work.

Personally, I think we should just merge this PR, as the current behavior is highly unexpected and gets in the way. I can’t think of a single scenario where you’d actually want the current behavior of invoking the click handlers after navigation already completed.

@bigopon
Copy link
Member

bigopon commented Aug 2, 2018

@thomas-darling For the reason preventDefault() is automatically called when click handler doesn't return true, it's a massive breaking change if we are to change this to capturing phase. Assume that aurelia-binding was created before aurelia-router, it makes sense why the default behavior is to listen to click during capturing phase. So... I think unless we move aurelia-framework to 2.0, this original behavior will stay for awhile.

@thomas-darling
Copy link
Author

thomas-darling commented Aug 2, 2018

I’m not advocating for changing that, and it’s not part of this PR - I mentioned it, but I think the current behavior is acceptable.

The only thing breaking if we merge this, is that any link that has both anhref and a handler, will now need to explicitly return true in the handler - otherwise the link won’t trigger routing when clicked.

Given how unlikely it is that anyone relies on the current behavior - where the handler runs after navigation completes - I doubt this would cause any real-world issues. And it should be simple to fix by adding return true to any handlers that might need it.

@bigopon
Copy link
Member

bigopon commented Aug 2, 2018

Given how unlikely it is that anyone relies on the current behavior

Yes that's true, I didn't think of this, note that it also breaks when an ancestor of the anchor declares click handler from template, but I doubt that we would see it in ... many apps.

@indfnzo
Copy link

indfnzo commented Mar 3, 2019

Same issue here, though I'm using Swiper which overrides clicks on links to prevent navigation when dragging.

The above solution doesn't work for my case since Swiper's event is non-capturing.

For what it's worth, I had to override the entire History implementation with my own (since for some reason I couldn't just inject a custom LinkHandler on the built-in BrowserHistory). Here's the code I used:

history-browser.ts
import {History} from 'aurelia-history';
import {LinkHandler} from 'aurelia-history-browser';
import {DOM, PLATFORM} from 'aurelia-pal';
import {DefaultPreventableLinkHandler} from './link-handler';

/**
 * Overrides the default BrowserHistory implementation from aurelia-history-browser.
 * See: https://github.com/aurelia/history-browser/pull/32
 */
export function configure(config): void {
  config.singleton(History, DefaultPreventableBrowserHistory);
  config.transient(LinkHandler, DefaultPreventableLinkHandler);
}

/**
 * An implementation of the basic history API.
 */
export class DefaultPreventableBrowserHistory extends History {
  static inject = [LinkHandler];

  _isActive;
  _checkUrlCallback;
  _wantsHashChange;
  _hasPushState;
  location;
  history;
  linkHandler;
  options;
  root;
  fragment;

  /**
   * Creates an instance of BrowserHistory
   * @param linkHandler An instance of LinkHandler.
   */
  constructor(linkHandler: LinkHandler) {
	super();

	this._isActive = false;
	this._checkUrlCallback = this._checkUrl.bind(this);

	this.location = PLATFORM.location;
	this.history = PLATFORM.history;
	this.linkHandler = linkHandler;
  }

  /**
   * Activates the history object.
   * @param options The set of options to activate history with.
   * @returns Whether or not activation occurred.
   */
  activate(options: any = {}): boolean {
	if (this._isActive) {
		throw new Error('History has already been activated.');
	}

	const wantsPushState = !!options.pushState;

	this._isActive = true;
	this.options = Object.assign({}, { root: '/' }, this.options, options);

	// Normalize root to always include a leading and trailing slash.
	this.root = ('/' + this.options.root + '/').replace(rootStripper, '/');

	this._wantsHashChange = this.options.hashChange !== false;
	this._hasPushState = !!(this.options.pushState && this.history && this.history.pushState);

	// Determine how we check the URL state.
	let eventName;
	if (this._hasPushState) {
		eventName = 'popstate';
	} else if (this._wantsHashChange) {
		eventName = 'hashchange';
	}

	PLATFORM.addEventListener(eventName, this._checkUrlCallback);

	// Determine if we need to change the base url, for a pushState link
	// opened by a non-pushState browser.
	if (this._wantsHashChange && wantsPushState) {
		// Transition from hashChange to pushState or vice versa if both are requested.
		const loc = this.location;
		const atRoot = loc.pathname.replace(/[^\/]$/, '$&/') === this.root;

		// If we've started off with a route from a `pushState`-enabled
		// browser, but we're currently in a browser that doesn't support it...
		if (!this._hasPushState && !atRoot) {
		this.fragment = this._getFragment(null, true);
		this.location.replace(this.root + this.location.search + '#' + this.fragment);
		// Return immediately as browser will do redirect to new url
		return true;

		// Or if we've started out with a hash-based route, but we're currently
		// in a browser where it could be `pushState`-based instead...
		} else if (this._hasPushState && atRoot && loc.hash) {
		this.fragment = this._getHash().replace(routeStripper, '');
		this.history.replaceState({}, DOM.title, this.root + this.fragment + loc.search);
		}
	}

	if (!this.fragment) {
		this.fragment = this._getFragment();
	}

	this.linkHandler.activate(this);

	if (!this.options.silent) {
		return this._loadUrl();
	}
  }

  /**
   * Deactivates the history object.
   */
  deactivate(): void {
	PLATFORM.removeEventListener('popstate', this._checkUrlCallback);
	PLATFORM.removeEventListener('hashchange', this._checkUrlCallback);
	this._isActive = false;
	this.linkHandler.deactivate();
  }

  /**
   * Returns the fully-qualified root of the current history object.
   * @returns The absolute root of the application.
   */
  getAbsoluteRoot(): string {
	const origin = createOrigin(this.location.protocol, this.location.hostname, this.location.port);
	return `${origin}${this.root}`;
  }

  /**
   * Causes a history navigation to occur.
   *
   * @param fragment The history fragment to navigate to.
   * @param options The set of options that specify how the navigation should occur.
   * @return Promise if triggering navigation, otherwise true/false indicating if navigation occurred.
   */
  navigate(fragment?: string, {trigger = true, replace = false} = {}): boolean {
	if (fragment && absoluteUrl.test(fragment)) {
		this.location.href = fragment;
		return true;
	}

	if (!this._isActive) {
		return false;
	}

	fragment = this._getFragment(fragment || '');

	if (this.fragment === fragment && !replace) {
		return false;
	}

	this.fragment = fragment;

	let url = this.root + fragment;

	// Don't include a trailing slash on the root.
	if (fragment === '' && url !== '/') {
		url = url.slice(0, -1);
	}

	// If pushState is available, we use it to set the fragment as a real URL.
	if (this._hasPushState) {
		url = url.replace('//', '/');
		this.history[replace ? 'replaceState' : 'pushState']({}, DOM.title, url);
	} else if (this._wantsHashChange) {
		// If hash changes haven't been explicitly disabled, update the hash
		// fragment to store history.
		updateHash(this.location, fragment, replace);
	} else {
		// If you've told us that you explicitly don't want fallback hashchange-
		// based history, then `navigate` becomes a page refresh.
		this.location.assign(url);
	}

	if (trigger) {
		return this._loadUrl(fragment);
	}

	return true;
  }

  /**
   * Causes the history state to navigate back.
   */
  navigateBack(): void {
	this.history.back();
  }

  /**
   * Sets the document title.
   */
  setTitle(title: string): void {
	DOM.title = title;
  }

  /**
   * Sets a key in the history page state.
   * @param key The key for the value.
   * @param value The value to set.
   */
  setState(key: string, value: any): void {
	const state = Object.assign({}, this.history.state);
	const { pathname, search, hash } = this.location;
	state[key] = value;
	this.history.replaceState(state, null, `${pathname}${search}${hash}`);
  }

  /**
   * Gets a key in the history page state.
   * @param key The key for the value.
   * @return The value for the key.
   */
  getState(key: string): any {
	const state = Object.assign({}, this.history.state);
	return state[key];
  }

  /**
   * Returns the current index in the navigation history.
   * @returns The current index.
   */
  getHistoryIndex(): number {
	let historyIndex = this.getState('HistoryIndex');
	if (historyIndex === undefined) {
		historyIndex = this.history.length - 1;
		this.setState('HistoryIndex', historyIndex);
	}
	return historyIndex;
  }

  /**
   * Move to a specific position in the navigation history.
   * @param movement The amount of steps, positive or negative, to move.
   */
  go(movement: number): void {
	this.history.go(movement);
  }

  _getHash(): string {
	return this.location.hash.substr(1);
  }

  _getFragment(fragment?: string, forcePushState?: boolean): string {
	let root;

	if (!fragment) {
		if (this._hasPushState || !this._wantsHashChange || forcePushState) {
		fragment = this.location.pathname + this.location.search;
		root = this.root.replace(trailingSlash, '');
		if (!fragment.indexOf(root)) {
			fragment = fragment.substr(root.length);
		}
		} else {
		fragment = this._getHash();
		}
	}

	return '/' + fragment.replace(routeStripper, '');
  }

  _checkUrl() {
	const current = this._getFragment();
	if (current !== this.fragment) {
		this._loadUrl();
	}
  }

  _loadUrl(fragmentOverride?: string): boolean {
	const fragment = this.fragment = this._getFragment(fragmentOverride);

	return this.options.routeHandler ?
		this.options.routeHandler(fragment) :
		false;
  }
}

// Cached regex for stripping a leading hash/slash and trailing space.
const routeStripper = /^#?\/*|\s+$/g;

// Cached regex for stripping leading and trailing slashes.
const rootStripper = /^\/+|\/+$/g;

// Cached regex for removing a trailing slash.
const trailingSlash = /\/$/;

// Cached regex for detecting if a URL is absolute,
// i.e., starts with a scheme or is scheme-relative.
// See http://www.ietf.org/rfc/rfc2396.txt section 3.1 for valid scheme format
const absoluteUrl = /^([a-z][a-z0-9+\-.]*:)?\/\//i;

// Update the hash location, either replacing the current entry, or adding
// a new one to the browser history.
function updateHash(location, fragment, replace) {
  if (replace) {
	const href = location.href.replace(/(javascript:|#).*$/, '');
	location.replace(href + '#' + fragment);
  } else {
	// Some browsers require that `hash` contains a leading #.
	location.hash = '#' + fragment;
  }
}

function createOrigin(protocol: string, hostname: string, port: string) {
  return `${protocol}//${hostname}${port ? ':' + port : ''}`;
}
link-handler.ts
import { AnchorEventInfo, LinkHandler } from 'aurelia-history-browser';
import { DOM, PLATFORM } from 'aurelia-pal';

/**
 * Overrides the default LinkHandler implementation to prevent eager navigation.
 * See: https://github.com/aurelia/history-browser/pull/32
 */
export class DefaultPreventableLinkHandler extends LinkHandler {

	/**
	 * Gets the href and a "should handle" recommendation, given an Event.
	 *
	 * @param event The Event to inspect for target anchor and href.
	 */
	static getEventInfo(event): AnchorEventInfo {
		const info = {
			shouldHandleEvent: false,
			href: null,
			anchor: null
		};

		if (event.defaultPrevented) {
			return info;
		}

		const target = DefaultPreventableLinkHandler.findClosestAnchor(event.target);
		if (!target || !DefaultPreventableLinkHandler.targetIsThisWindow(target)) {
			return info;
		}

		if (target.hasAttribute('download') || target.hasAttribute('router-ignore') || target.hasAttribute('data-router-ignore')) {
			return info;
		}

		if (event.altKey || event.ctrlKey || event.metaKey || event.shiftKey) {
			return info;
		}

		const href = target.getAttribute('href');
		info.anchor = target;
		info.href = href;

		const leftButtonClicked = event.which === 1;
		const isRelative = href && !(href.charAt(0) === '#' || (/^[a-z]+:/i).test(href));

		info.shouldHandleEvent = leftButtonClicked && isRelative;
		return info;
	}

	/**
	 * Finds the closest ancestor that's an anchor element.
	 *
	 * @param el The element to search upward from.
	 * @returns The link element that is the closest ancestor.
	 */
	static findClosestAnchor(el: Element): Element {
	while (el) {
		if (el.tagName === 'A') {
			return el;
		}

		el = el.parentNode as Element;
	}
	}

	/**
	 * Gets a value indicating whether or not an anchor targets the current window.
	 *
	 * @param target The anchor element whose target should be inspected.
	 * @returns True if the target of the link element is this window; false otherwise.
	 */
	static targetIsThisWindow(target: Element): boolean {
	  const targetWindow = target.getAttribute('target');
	  const win = PLATFORM.global;

	  return !targetWindow ||
		targetWindow === win.name ||
		targetWindow === '_self';
	}
	handler;
	history;

	/**
	 * Creates an instance of DefaultLinkHandler.
	 */
	constructor() {
	  super();

	  this.handler = (e) => {
		const {shouldHandleEvent, href} = DefaultPreventableLinkHandler.getEventInfo(e);

		if (shouldHandleEvent) {
		  e.preventDefault();
		  this.history.navigate(href);
		}
	  };
	}

	/**
	 * Activate the instance.
	 *
	 * @param history The BrowserHistory instance that navigations should be dispatched to.
	 */
	activate(history): void {
	  if (history._hasPushState) {
		this.history = history;
		DOM.addEventListener('click', this.handler, false);
	  }
	}

	/**
	 * Deactivate the instance. Event handlers and other resources should be cleaned up here.
	 */
	deactivate(): void {
	  DOM.removeEventListener('click', this.handler, false);
	}
  }

I use the above as my history implementation from main.ts:

export async function configure(aurelia: Aurelia) {
	aurelia.use
		.basicConfiguration()
		.router()
		.plugin(PLATFORM.moduleName('history-browser'));
}

It took way too long for me to get this working right, so it would be great if this PR would be merged!


As a side note, I have tried creating a repro environment on stack blitz, but the latest aurelia-script bundle doesn't have this issue anymore.

@thomas-darling
Copy link
Author

Yep, I'd really like to see this one merged too - the current behavior must be super confusing for new users, and hacking around it is indeed tricky, at least until you figure out what's going on.
What do you say, @EisenbergEffect? :-)

Here's the hack I include in literally every app I build:

// tslint:disable: no-invalid-this no-unbound-method

import { DOM } from "aurelia-framework";
import { DefaultLinkHandler, AnchorEventInfo } from "aurelia-history-browser";

// TODO: Remove this when https://github.com/aurelia/history-browser/pull/32 is released.

/**
 * HACK: The `DefaultLinkHandler` attaches a click listener to the document element, which
 * ensures a click on any anchor element activates the router. Unfortunately, it listens
 * for this event in the `capture` phase, which means no other handlers will ever get a
 * chance to handle the event.
 * Therefore, we override the `activate` method to change the event listener to listen
 * for the event in the `bubble` phase instead.
 */
DefaultLinkHandler.prototype.activate = function(history: any): void
{
    if (history._hasPushState)
    {
        (this as any).history = history;
        DOM.addEventListener("click", (this as any).handler, false);
    }
};

/**
 * HACK: The `DefaultLinkHandler` does not consider whether `preventDefault()` has been
 * called on the event, which means it is impossible to prevent the route change.
 * Therefore, we override the static `getEventInfo` method to change the behavior,
 * such that events are not handled by the router if default has been prevented.
 */
const getEventInfoFunc = DefaultLinkHandler.getEventInfo;
DefaultLinkHandler.getEventInfo = function(event: Event): AnchorEventInfo
{
    if (event.defaultPrevented)
    {
        return { shouldHandleEvent: false, href: null, anchor: null } as any;
    }

    // Continue with the original method.
    return getEventInfoFunc.apply(this, arguments as any);
};

On a side note, @jemhuntr, what do you mean by this?

the latest aurelia-script bundle doesn't have this issue anymore.

I just had a look in the code, and the bugs are stil there, so... :-)

@indfnzo
Copy link

indfnzo commented Mar 3, 2019

That looks sooo much cleaner than having to replace the entire BrowserHistory's implementation :)))

On a side note, @jemhuntr, what do you mean by this?

the latest aurelia-script bundle doesn't have this issue anymore.

I just had a look in the code, and the bugs are stil there, so... :-)

I spent some time trying to create a repro at https://codesandbox.io/s/wox4mzyv5l (CodeSandbox, not StackBlitz, sorry). I couldn't reproduce the issue since Swiper is able to override the clicks just fine.

EDIT: Apparently CodeSandbox includes a link handler at sse-hooks.js which handles navigation and event propagation. The aurelia function is unmodified so without CodeSandbox overriding that, it couldn't have possibly worked :)))

@thomas-darling
Copy link
Author

Haha, yeah, though you might like it a little better :-)

The way I usually manage hacks like this, is to put each in its own file, and then import them into main.ts with something like import "./hacks/fix-link-handling.ts". Alternatively, you can use patch-package to apply Git patch files to packages after they are installed.

Oh, and it makes sense that this works in your CodeSandbox - there you are loading the Swiper script before the Aurelia script, which means it gets to attach its own handler first ;-)

@EisenbergEffect
Copy link
Contributor

Could we put the new behavior behind a setting? We could use that to maintain backward compatibility while also enabling people who need this to get it working easily. We could then port this forward to vNext so we don't have this issue in the future and it becomes the standard behavior. Thoughts? @thomas-darling What do you think about putting this behind a history config setting?

@thomas-darling
Copy link
Author

thomas-darling commented Mar 4, 2019

@EisenbergEffect We can definitely do that for vCurrent, to preserve backwards compatibility :-)

I don't think we should have that setting in vNext though - there it should just behave correctly.
That's also what you were suggesting, right?

Regarding implementation:

  • What should we name this option?
    Off the top of my head, I'd suggest history.useStandardLinkHandling, as allowing the event to bubble and respecting preventDefault is in fact the standard behavior in a plain HTML document. Alternatively we could put it inside an options object, but it seems a little pointless when it's the only option, and something that will be removed in vNext anyway.

  • Should we enable or disable this by default?
    It's a breaking change, so that suggests it should be disabled - but then we need to communicate to people that this setting exists, and should be enabled. Alternatively we can enable it and communicate to people that they can disable it if things break. Your choice :-)

  • Should we put this on History or BrowserHistory?
    It's an issue that's unique to BrowserHistory, but since the key used for DI is History, and since the idea is to use a platform-specific implementation, I guess it should also be defined as an optional property on History. Agree?

    That way it can be set with

    router.history.useStandardLinkHandling = true

    in the configureRouter method of the root component of the app.

I can update the pull request with the changes sometime during this week :-)

@EisenbergEffect
Copy link
Contributor

The name useStandardLinkHandling is ok. Let's just make sure we have some code comment docs there so it gets generated into the site documentation, and also update the article on configuring the router. I think this should be off by default, to preserve backwards compatibility. We can make this the norm for vNext, and remove the setting. It's a bit odd to have this on the abstract History class though. Should we introduce a generic way to set options on History such that particular implementations can interpret them in implementation-specific ways?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants