fix(RouterLink): do not prevent default behavior if target set on anchor element

jeffbcross · jeffbcross · commit a69e7fe297d9 · 2015-11-04T19:55:46.000Z
If the anchor element on which the "router-link" directive is present has a target attribute other than "_self," the handler will not prevent default behavior of the browser. Closes #4233 Closes #5082
diff --git a/modules/angular2/src/router/router_link.ts b/modules/angular2/src/router/router_link.ts
@@ -1,5 +1,5 @@
 import {Directive} from '../core/metadata';
-import {StringMapWrapper} from 'angular2/src/core/facade/collection';
+import {isString} from 'angular2/src/core/facade/lang';
 
 import {Router} from './router';
 import {Location} from './location';
@@ -36,7 +36,7 @@ import {Instruction, stringifyInstruction} from './instruction';
  */
 @Directive({
   selector: '[router-link]',
-  inputs: ['routeParams: routerLink'],
+  inputs: ['routeParams: routerLink', 'target: target'],
   host: {
     '(click)': 'onClick()',
     '[attr.href]': 'visibleHref',
@@ -48,6 +48,7 @@ export class RouterLink {
 
   // the url displayed on the anchor element.
   visibleHref: string;
+  target: string;
 
   // the instruction passed to the router to navigate
   private _navigationInstruction: Instruction;
@@ -65,7 +66,11 @@ export class RouterLink {
   }
 
   onClick(): boolean {
-    this._router.navigateByInstruction(this._navigationInstruction);
-    return false;
+    // If no target, or if target is _self, prevent default browser behavior
+    if (!isString(this.target) || this.target == '_self') {
+      this._router.navigateByInstruction(this._navigationInstruction);
+      return false;
+    }
+    return true;
   }
 }
diff --git a/modules/angular2/test/router/router_link_spec.ts b/modules/angular2/test/router/router_link_spec.ts
@@ -56,7 +56,8 @@ export function main() {
          tcb.createAsync(TestComponent)
              .then((testComponent) => {
                testComponent.detectChanges();
-               let anchorElement = testComponent.debugElement.query(By.css('a')).nativeElement;
+               let anchorElement =
+                   testComponent.debugElement.query(By.css('a.detail-view')).nativeElement;
                expect(DOM.getAttribute(anchorElement, 'href')).toEqual('detail');
                async.done();
              });
@@ -70,11 +71,38 @@ export function main() {
              .then((testComponent) => {
                testComponent.detectChanges();
                // TODO: shouldn't this be just 'click' rather than '^click'?
-               testComponent.debugElement.query(By.css('a')).triggerEventHandler('click', null);
+               testComponent.debugElement.query(By.css('a.detail-view'))
+                   .triggerEventHandler('click', null);
                expect(router.spy('navigateByInstruction')).toHaveBeenCalledWith(dummyInstruction);
                async.done();
              });
        }));
+
+    it('should call router.navigate when a link is clicked if target is _self',
+       inject([AsyncTestCompleter, Router], (async, router) => {
+
+         tcb.createAsync(TestComponent)
+             .then((testComponent) => {
+               testComponent.detectChanges();
+               testComponent.debugElement.query(By.css('a.detail-view-self'))
+                   .triggerEventHandler('click', null);
+               expect(router.spy('navigateByInstruction')).toHaveBeenCalledWith(dummyInstruction);
+               async.done();
+             });
+       }));
+
+    it('should NOT call router.navigate when a link is clicked if target is set to other than _self',
+       inject([AsyncTestCompleter, Router], (async, router) => {
+
+         tcb.createAsync(TestComponent)
+             .then((testComponent) => {
+               testComponent.detectChanges();
+               testComponent.debugElement.query(By.css('a.detail-view-blank'))
+                   .triggerEventHandler('click', null);
+               expect(router.spy('navigateByInstruction')).not.toHaveBeenCalled();
+               async.done();
+             });
+       }));
   });
 }
 
@@ -94,7 +122,20 @@ class UserCmp {
 @View({
   template: `
     <div>
-      <a [router-link]="['/Detail']">detail view</a>
+      <a [router-link]="['/Detail']"
+         class="detail-view">
+           detail view
+      </a>
+      <a [router-link]="['/Detail']"
+         class="detail-view-self"
+         target="_self">
+           detail view with _self target
+      </a>
+      <a [router-link]="['/Detail']"
+         class="detail-view-blank"
+         target="_blank">
+           detail view with _blank target
+      </a>
     </div>`,
   directives: [RouterLink]
 })
diff --git a/modules/playground/e2e_test/hash_routing/hash_location_spec.ts b/modules/playground/e2e_test/hash_routing/hash_location_spec.ts
@@ -26,4 +26,19 @@ describe('hash routing example app', function() {
 
     expect(element(by.css('goodbye-cmp')).getText()).toContain('goodbye');
   });
+
+
+  it('should open in new window if target is _blank', () => {
+    var URL = 'playground/src/hash_routing/index.html';
+    browser.get(URL + '#/');
+    waitForElement('hello-cmp');
+
+    element(by.css('#goodbye-link-blank')).click();
+    expect(browser.driver.getCurrentUrl()).not.toContain('#/bye');
+    browser.getAllWindowHandles().then(function(windows) {
+      browser.switchTo()
+          .window(windows[1])
+          .then(function() { expect(browser.driver.getCurrentUrl()).toContain("#/bye"); });
+    });
+  });
 });
diff --git a/modules/playground/src/hash_routing/index.ts b/modules/playground/src/hash_routing/index.ts
@@ -29,6 +29,9 @@ class GoodByeCmp {
     <nav>
       <a href="#/" id="hello-link">Navigate via href</a> |
       <a [router-link]="['/GoodbyeCmp']" id="goodbye-link">Navigate with Link DSL</a>
+      <a [router-link]="['/GoodbyeCmp']" id="goodbye-link-blank" target="_blank">
+        Navigate with Link DSL _blank target
+      </a>
     </nav>
     <router-outlet></router-outlet>
   `,
