Fix review comments

Author: youda97

src/dropdown/dropdown.component.ts

@@ -305,10 +305,10 @@ export class Dropdown implements OnInit, AfterContentInit, OnDestroy {
 			this.dropdownButton.nativeElement.focus();
 		} else if (this.menuIsClosed && (event.key === " " || event.key === "ArrowDown" || event.key === "ArrowUp" ||
 			event.key === "Spacebar" || event.key === "Down" || event.key === "Up")) {
-			event.preventDefault();
 			if (this.disableArrowKeys && (event.key === "ArrowDown" || event.key === "ArrowUp" || event.key === "Down" || event.key === "Up")) {
 				return;
 			}
+			event.preventDefault();
 			this.openMenu();
 		}
 

src/table/data-grid-focus.directive.ts

@@ -2,7 +2,9 @@ import {
 	Directive,
 	Input,
 	ElementRef,
-	HostListener
+	HostListener,
+	Output,
+	EventEmitter
 } from "@angular/core";
 import { Table } from "./table.component";
 import { getFocusElementList, tabbableSelectorIgnoreTabIndex } from "../common/tab.service";
@@ -11,21 +13,32 @@ import { setTabIndex } from "../utils/a11y";
 @Directive({
 	selector: "[ibmDataGridFocus]"
 })
-
 export class DataGridFocus {
 	@Input() ibmDataGridFocus: boolean;
-	@Input() columnIndex: number;
+	@Input() set columnIndex(value: number) {
+		const shouldEmit = value !== this._columnIndex;
+		this._columnIndex = value;
+		if (shouldEmit) {
+			this.columnIndexChange.emit(value);
+		}
+	}
+	get columnIndex(): number {
+		return this._columnIndex;
+	}
+	@Output() columnIndexChange: EventEmitter<number> = new EventEmitter();
 
-	constructor(private elementRef: ElementRef) {}
+	protected _columnIndex: number;
 
-	focus(focusElement) {
-		const focusElementList = getFocusElementList(focusElement, tabbableSelectorIgnoreTabIndex);
-		if (focusElement.firstElementChild && focusElement.firstElementChild.classList.contains("bx--table-sort-v2")) {
+	constructor(protected elementRef: ElementRef) {}
+
+	focus(element) {
+		const focusElementList = getFocusElementList(element, tabbableSelectorIgnoreTabIndex);
+		if (element.firstElementChild && element.firstElementChild.classList.contains("bx--table-sort-v2")) {
 			focusElementList[1].focus();
 		} else if (focusElementList.length > 0) {
 			focusElementList[0].focus();
 		}  else {
-			focusElement.focus();
+			element.focus();
 		}
 	}
 
@@ -36,12 +49,16 @@ export class DataGridFocus {
 		}
 		const element = this.elementRef.nativeElement;
 		const rows = element.closest("table").rows;
-		let rowIndex = Array.from(rows).findIndex(item => item === element.closest("tr"));
+		const closestTr = element.closest("tr");
+		let rowIndex = Array.from(rows).indexOf(closestTr);
+
+		const headerRow = rows[0].querySelectorAll("th");
 
 		switch (event.key) {
 			case "Right": // IE specific value
 			case "ArrowRight":
-				if (element.nextElementSibling && element.nextElementSibling.style.width !== "0px") {
+				if (element.nextElementSibling && Array.from(headerRow).indexOf(element.nextElementSibling) < headerRow.length - 1) {
+					this.columnIndex++;
 					const nextSibling = element.nextElementSibling;
 					setTabIndex(element, -1);
 					setTabIndex(nextSibling, 0);
@@ -51,6 +68,7 @@ export class DataGridFocus {
 			case "Left": // IE specific value
 			case "ArrowLeft":
 				if (element.previousElementSibling) {
+					this.columnIndex--;
 					const previousSibling = element.previousElementSibling;
 					setTabIndex(element, -1);
 					setTabIndex(previousSibling, 0);
@@ -63,7 +81,7 @@ export class DataGridFocus {
 					rowIndex++;
 					const row = rows[rowIndex].querySelectorAll("td");
 					setTabIndex(element, -1);
-					if (rows[rowIndex].className === "bx--expandable-row-v2") {
+					if (rows[rowIndex].classList.item(0) === "bx--expandable-row-v2") {
 						setTabIndex(row[0], 0);
 						this.focus(row[0]);
 					} else {
@@ -74,27 +92,25 @@ export class DataGridFocus {
 				break;
 			case "Up": // IE specific value
 			case "ArrowUp":
-				const headerRow = rows[0].querySelectorAll("th");
+				if ((rowIndex !== 1 && Array.from(headerRow).some(th => getFocusElementList(th, tabbableSelectorIgnoreTabIndex).length < 0)) ||
+					rowIndex === 0) {
+						return;
+				}
 				setTabIndex(element, -1);
-				if (rowIndex === 1 && Array.from(headerRow).some(th => getFocusElementList(th, tabbableSelectorIgnoreTabIndex).length > 0)) {
-					setTabIndex(headerRow[this.columnIndex], 0);
-					this.focus(headerRow[this.columnIndex]);
-				} else if (rowIndex > 1) {
-					rowIndex--;
-					const row = rows[rowIndex].querySelectorAll("td");
-					if (rows[rowIndex].className === "bx--expandable-row-v2") {
-						setTabIndex(row[0], 0);
-						this.focus(row[0]);
-					} else {
-						setTabIndex(row[this.columnIndex], 0);
-						this.focus(row[this.columnIndex]);
-					}
+				rowIndex--;
+				const row = rows[rowIndex].querySelectorAll("td, th");
+				if (rows[rowIndex].classList.item(0) === "bx--expandable-row-v2") {
+					setTabIndex(row[0], 0);
+					this.focus(row[0]);
+				} else {
+					setTabIndex(row[this.columnIndex], 0);
+					this.focus(row[this.columnIndex]);
 				}
 				break;
 			case "Home":
+				this.columnIndex = 0;
 				setTabIndex(element, -1);
 				if (event.ctrlKey) {
-					const headerRow = rows[0].querySelectorAll("th");
 					if (Array.from(headerRow).some(th => getFocusElementList(th, tabbableSelectorIgnoreTabIndex).length > 0)) {
 						setTabIndex(headerRow[0], 0);
 						this.focus(headerRow[0]);
@@ -103,7 +119,7 @@ export class DataGridFocus {
 						setTabIndex(firstBodyCell, 0);
 						this.focus(firstBodyCell);
 					}
-				} else if (element.parentElement.className !== "bx--expandable-row-v2") {
+				} else {
 					const firstRowCell = rows[rowIndex].querySelectorAll("th, td")[0];
 					setTabIndex(firstRowCell, 0);
 					this.focus(firstRowCell);
@@ -116,11 +132,11 @@ export class DataGridFocus {
 					this.columnIndex = lastRow.length - 1;
 					setTabIndex(lastRow[this.columnIndex], 0);
 					this.focus(lastRow[this.columnIndex]);
-				} else if (element.parentElement.className !== "bx--expandable-row-v2") {
-					this.columnIndex = lastRow.length - 1;
-					const lastRowCell = rows[rowIndex].querySelectorAll("th, td")[this.columnIndex];
-					setTabIndex(lastRowCell, 0);
-					this.focus(lastRowCell);
+				} else {
+					const currentRow = rows[rowIndex].querySelectorAll("th, td");
+					this.columnIndex = currentRow.length - 1;
+					setTabIndex(currentRow[this.columnIndex], 0);
+					this.focus(currentRow[this.columnIndex]);
 				}
 				break;
 		}

src/table/table.component.spec.ts

@@ -2,7 +2,13 @@ import { Component, OnInit } from "@angular/core";
 import { DialogModule } from "./../dialog/dialog.module";
 import { TestBed } from "@angular/core/testing";
 import { FormsModule } from "@angular/forms";
-import { TableModule, TableModel, TableHeaderItem, TableItem, DataGridFocus } from "./table.module";
+import {
+	TableModule,
+	TableModel,
+	TableHeaderItem,
+	TableItem,
+	DataGridFocus
+} from "./table.module";
 import { Table } from "./table.component";
 import { StaticIconModule } from "./../icon/static-icon.module";
 

src/table/table.component.ts

@@ -180,14 +180,12 @@ import { I18n } from "./../i18n/i18n.module";
 			<tr>
 				<th *ngIf="model.hasExpandableRows()"
 					[ibmDataGridFocus]="isDataGrid"
-					[columnIndex]="columnIndex"
-					(keyup)="handleInteractions($event)"
+					[(columnIndex)]="columnIndex"
 					(click)="setExpandIndex($event)">
 				</th>
 				<th *ngIf="!skeleton && showSelectionColumn"
 					[ibmDataGridFocus]="isDataGrid"
-					[columnIndex]="columnIndex"
-					(keyup)="handleInteractions($event)"
+					[(columnIndex)]="columnIndex"
 					(click)="setCheckboxIndex()"
 					style="width: 10px;">
 					<ibm-checkbox
@@ -205,11 +203,10 @@ import { I18n } from "./../i18n/i18n.module";
 					[class]="column.className"
 					[ngStyle]="column.style"
 					[ibmDataGridFocus]="isDataGrid"
-					[columnIndex]="columnIndex"
+					[(columnIndex)]="columnIndex"
 					[draggable]="columnsDraggable"
 					(dragstart)="columnDragStart($event, i)"
 					(dragend)="columnDragEnd($event, i)"
-					(keyup)="handleInteractions($event)"
 					(click)="setIndex($event, i)">
 						<span *ngIf="skeleton"></span>
 						<div
@@ -330,9 +327,8 @@ import { I18n } from "./../i18n/i18n.module";
 					*ngIf="model.hasExpandableRows()"
 					class="bx--table-expand-v2"
 					[ibmDataGridFocus]="isDataGrid"
-					[columnIndex]="columnIndex"
+					[(columnIndex)]="columnIndex"
 					[attr.data-previous-value]="(model.rowsExpanded[i] ? 'collapsed' : null)"
-					(keyup)="handleInteractions($event)"
 					(click)="setExpandIndex($event)">
 						<button
 						*ngIf="model.isRowExpandable(i)"
@@ -347,8 +343,7 @@ import { I18n } from "./../i18n/i18n.module";
 					<td
 						*ngIf="!skeleton && showSelectionColumn"
 						[ibmDataGridFocus]="isDataGrid"
-						[columnIndex]="columnIndex"
-						(keyup)="handleInteractions($event)"
+						[(columnIndex)]="columnIndex"
 						(click)="setCheckboxIndex()">
 						<ibm-checkbox
 							inline="true"
@@ -363,8 +358,7 @@ import { I18n } from "./../i18n/i18n.module";
 							[class]="model.header[j].className"
 							[ngStyle]="model.header[j].style"
 							[ibmDataGridFocus]="isDataGrid"
-							[columnIndex]="columnIndex"
-							(keyup)="handleInteractions($event)"
+							[(columnIndex)]="columnIndex"
 							(click)="setIndex($event, j)">
 							<ng-container *ngIf="!item.template">{{item.data}}</ng-container>
 							<ng-template
@@ -379,7 +373,7 @@ import { I18n } from "./../i18n/i18n.module";
 				[attr.data-child-row]="(model.rowsExpanded[i] ? 'true' : null)">
 					<td
 						[ibmDataGridFocus]="isDataGrid"
-						[columnIndex]="columnIndex"
+						[(columnIndex)]="columnIndex"
 						[attr.colspan]="row.length + 2"
 						(click)="setExpandIndex($event)">
 						<ng-container *ngIf="!firstExpandedTemplateInRow(row)">{{firstExpandedDataInRow(row)}}</ng-container>
@@ -482,6 +476,7 @@ export class Table implements AfterViewInit {
 		});
 		if (this.isDataGrid) {
 			this._model.rowsExpandedChange.subscribe(() => {
+				// Allows the expanded row to have a focus state when it exists in the DOM
 				setTimeout(() => {
 					const expandedRows = this.elementRef.nativeElement.querySelectorAll(".bx--expandable-row-v2:not(.bx--parent-row-v2)");
 					Array.from<any>(expandedRows).forEach(row => {
@@ -946,30 +941,6 @@ export class Table implements AfterViewInit {
 		}
 	}
 
-	// This is handled in the component so that the columnIndex is saved as you go into expandable rows
-	handleInteractions(event: KeyboardEvent) {
-		switch (event.key) {
-			case "Right": // IE specific value
-			case "ArrowRight":
-				if (this.columnIndex < this.getTotalColumns() - 1) {
-					this.columnIndex++;
-				}
-				break;
-			case "Left": // IE specific value
-			case "ArrowLeft":
-				if (this.columnIndex > 0) {
-					this.columnIndex--;
-				}
-				break;
-			case "Home":
-				this.columnIndex = 0;
-				break;
-			case "End":
-				this.columnIndex = this.getTotalColumns() - 1;
-				break;
-		}
-	}
-
 	setIndex(event, columnIndex) {
 		if (this.model.hasExpandableRows() && this.showSelectionColumn) {
 			this.columnIndex = columnIndex + 2;
@@ -989,13 +960,4 @@ export class Table implements AfterViewInit {
 	setExpandIndex(event) {
 		this.columnIndex = 0;
 	}
-
-	@HostListener("keyup", ["$event"])
-	keyUp(event: KeyboardEvent) {
-		if (event.key === "Home" && event.ctrlKey) {
-			this.columnIndex = 0;
-		} else if (event.key === "End" && event.ctrlKey) {
-			this.columnIndex = this.getTotalColumns() - 1;
-		}
-	}
 }