fix(table): data source should sort empty values correctly (#8698)

Author: andrewseguin

src/cdk/coercion/number-property.ts

@@ -10,8 +10,16 @@
 export function coerceNumberProperty(value: any): number;
 export function coerceNumberProperty<D>(value: any, fallback: D): number | D;
 export function coerceNumberProperty(value: any, fallbackValue = 0) {
+  return _isNumberValue(value) ? Number(value) : fallbackValue;
+}
+
+/**
+ * Whether the provided value is considered a number.
+ * @docs-private
+ */
+export function _isNumberValue(value: any): boolean {
   // parseFloat(value) handles most of the cases we're interested in (it treats null, empty string,
   // and other non-number values as NaN, where Number just uses 0) but it considers the string
   // '123hello' to be a valid number. Therefore we also check if Number(value) is NaN.
-  return isNaN(parseFloat(value as any)) || isNaN(Number(value)) ? fallbackValue : Number(value);
+  return !isNaN(parseFloat(value as any)) && !isNaN(Number(value));
 }

src/lib/table/table-data-source.ts

@@ -15,6 +15,7 @@ import {combineLatest} from 'rxjs/operators/combineLatest';
 import {map} from 'rxjs/operators/map';
 import {startWith} from 'rxjs/operators/startWith';
 import {empty} from 'rxjs/observable/empty';
+import {_isNumberValue} from '@angular/cdk/coercion';
 
 /**
  * Data source that accepts a client-side data array and includes native support of filtering,
@@ -88,7 +89,8 @@ export class MatTableDataSource<T> implements DataSource<T> {
   private _paginator: MatPaginator|null;
 
   /**
-   * Data accessor function that is used for accessing data properties for sorting.
+   * Data accessor function that is used for accessing data properties for sorting through
+   * the default sortData function.
    * This default function assumes that the sort header IDs (which defaults to the column name)
    * matches the data's properties (e.g. column Xyz represents data['Xyz']).
    * May be set to a custom function for different behavior.
@@ -98,21 +100,54 @@ export class MatTableDataSource<T> implements DataSource<T> {
   sortingDataAccessor: ((data: T, sortHeaderId: string) => string|number) =
       (data: T, sortHeaderId: string): string|number => {
     const value: any = data[sortHeaderId];
+    return _isNumberValue(value) ? Number(value) : value;
+  }
 
-    // If the value is a string and only whitespace, return the value.
-    // Otherwise +value will convert it to 0.
-    if (typeof value === 'string' && !value.trim()) {
-      return value;
-    }
+  /**
+   * Gets a sorted copy of the data array based on the state of the MatSort. Called
+   * after changes are made to the filtered data or when sort changes are emitted from MatSort.
+   * By default, the function retrieves the active sort and its direction and compares data
+   * by retrieving data using the sortingDataAccessor. May be overridden for a custom implementation
+   * of data ordering.
+   * @param data The array of data that should be sorted.
+   * @param sort The connected MatSort that holds the current sort state.
+   */
+  sortData: ((data: T[], sort: MatSort) => T[]) = (data: T[], sort: MatSort): T[] => {
+    const active = sort.active;
+    const direction = sort.direction;
+    if (!active || direction == '') { return data; }
+
+    return data.sort((a, b) => {
+      let valueA = this.sortingDataAccessor(a, active);
+      let valueB = this.sortingDataAccessor(b, active);
 
-    return isNaN(+value) ? value : +value;
+      // If both valueA and valueB exist (truthy), then compare the two. Otherwise, check if
+      // one value exists while the other doesn't. In this case, existing value should come first.
+      // This avoids inconsistent results when comparing values to undefined/null.
+      // If neither value exists, return 0 (equal).
+      let comparatorResult = 0;
+      if (valueA && valueB) {
+        // Check if one value is greater than the other; if equal, comparatorResult should remain 0.
+        if (valueA > valueB) {
+          comparatorResult = 1;
+        } else if (valueA < valueB) {
+          comparatorResult = -1;
+        }
+      } else if (valueA) {
+        comparatorResult = 1;
+      } else if (valueB) {
+        comparatorResult = -1;
+      }
+
+      return comparatorResult * (direction == 'asc' ? 1 : -1);
+    });
   }
 
   /**
    * Checks if a data object matches the data source's filter string. By default, each data object
    * is converted to a string of its properties and returns true if the filter has
    * at least one occurrence in that string. By default, the filter string has its whitespace
-   * trimmed and the match is case-insensitive. May be overriden for a custom implementation of
+   * trimmed and the match is case-insensitive. May be overridden for a custom implementation of
    * filter matching.
    * @param data Data object used to check against the filter.
    * @param filter Filter string that has been set on the data source.
@@ -172,7 +207,7 @@ export class MatTableDataSource<T> implements DataSource<T> {
   _filterData(data: T[]) {
     // If there is a filter string, filter out data that does not contain it.
     // Each data object is converted to a string using the function defined by filterTermAccessor.
-    // May be overriden for customization.
+    // May be overridden for customization.
     this.filteredData =
         !this.filter ? data : data.filter(obj => this.filterPredicate(obj, this.filter));
 
@@ -188,16 +223,9 @@ export class MatTableDataSource<T> implements DataSource<T> {
    */
   _orderData(data: T[]): T[] {
     // If there is no active sort or direction, return the data without trying to sort.
-    if (!this.sort || !this.sort.active || this.sort.direction == '') { return data; }
-
-    const active = this.sort.active;
-    const direction = this.sort.direction;
+    if (!this.sort) { return data; }
 
-    return data.slice().sort((a, b) => {
-      let valueA = this.sortingDataAccessor(a, active);
-      let valueB = this.sortingDataAccessor(b, active);
-      return (valueA < valueB ? -1 : 1) * (direction == 'asc' ? 1 : -1);
-    });
+    return this.sortData(data.slice(), this.sort);
   }
 
   /**

src/lib/table/table.spec.ts

@@ -209,6 +209,42 @@ describe('MatTable', () => {
         ['a_2', 'b_2', 'c_2'],
         ['a_3', 'b_3', 'c_3'],
       ]);
+
+      // Expect that empty string row comes before the other values
+      component.sort.sort(component.sortHeader);
+      fixture.detectChanges();
+      expectTableToMatchContent(tableElement, [
+        ['Column A\xa0Sorted by a descending', 'Column B', 'Column C'],
+        ['a_3', 'b_3', 'c_3'],
+        ['a_2', 'b_2', 'c_2'],
+        ['', 'b_1', 'c_1'],
+      ]);
+    });
+
+    it('should by default correctly sort undefined values', () => {
+      // Activate column A sort
+      dataSource.data[0].a = undefined;
+
+      // Expect that undefined row comes before the other values
+      component.sort.sort(component.sortHeader);
+      fixture.detectChanges();
+      expectTableToMatchContent(tableElement, [
+        ['Column A\xa0Sorted by a ascending', 'Column B', 'Column C'],
+        ['', 'b_1', 'c_1'],
+        ['a_2', 'b_2', 'c_2'],
+        ['a_3', 'b_3', 'c_3'],
+      ]);
+
+
+      // Expect that undefined row comes after the other values
+      component.sort.sort(component.sortHeader);
+      fixture.detectChanges();
+      expectTableToMatchContent(tableElement, [
+        ['Column A\xa0Sorted by a descending', 'Column B', 'Column C'],
+        ['a_3', 'b_3', 'c_3'],
+        ['a_2', 'b_2', 'c_2'],
+        ['', 'b_1', 'c_1'],
+      ]);
     });
 
     it('should be able to page the table contents', fakeAsync(() => {
@@ -243,9 +279,9 @@ describe('MatTable', () => {
 });
 
 interface TestData {
-  a: string;
-  b: string;
-  c: string;
+  a: string|undefined;
+  b: string|undefined;
+  c: string|undefined;
 }
 
 class FakeDataSource extends DataSource<TestData> {