Improve test freshness page interaction experience.
Created attachment 380405 [details] Patch
Created attachment 380454 [details] Patch
Created attachment 380455 [details] Patch
Comment on attachment 380455 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380455&action=review > Websites/perf.webkit.org/public/v3/components/freshness-indicator.js:24 > + container.onmouseenter = this.createEventHandler(() => this.dispatchAction('select', this)); Use mouseover? > Websites/perf.webkit.org/public/v3/components/freshness-indicator.js:29 > + container.onclick = this.createEventHandler(() => this.dispatchAction('click', this)); > + container.onfocus = this.createEventHandler(() => { > + if (!mouseInUse) > + this.dispatchAction('focus', this); Things like click and focus shouldn't be an action. Maybe "toggle"? > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:67 > + window.addEventListener('keydown', (event) => { All these event listeners should only come to effect when one of the indicators are in focus. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:69 > + this._builderByIndicator.keys().next().value.focus({preventScroll: true}); We should probably add a helper function or a local variable to say this is the first indicator. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:79 > + } > + else if (event.code == 'ArrowDown') { Nit: wrong style. } should come before else if. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:83 > + } > + else if (event.code == 'ArrowLeft') Ditto. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:84 > + this._findAndUpdateClosestIndicator(column - 1, row, this._indicatorGrid, this.constructor.LEFT_ONLY); Just use a string like 'left' instead of a LEFT_ONLY. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:86 > + this._findAndUpdateClosestIndicator(column + 1, row, this._indicatorGrid, this.constructor.RIGHT_ONLY); Ditto. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:90 > + else if (event.code == 'Enter') { > + this._showTooltip = !this._showTooltip; > + this.enqueueToRender(); > + } else if (event.code == 'Escape') { I don't think we should override the behavior of enter key. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:91 > + event.preventDefault(); This should only be listened be on an indicator and maybe on the tooltip itself. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:118 > + let offSet = 1; Should be "offset". > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:326 > + this._indicatorForTooltip = this._hoveringIndicator = originator; Nit: Wrong style. Each assignment needs to happen on its own line. https://webkit.org/code-style-guidelines/#linebreaking-multiple-statements
Created attachment 380491 [details] Patch
Comment on attachment 380491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380491&action=review > Websites/perf.webkit.org/public/v3/components/base.js:253 > + createEventHandler(callback, preventDefault=true, stopPropagation=true) { return ComponentBase.createEventHandler(callback, preventDefault, stopPropagation); } > + static createEventHandler(callback, preventDefault=true, stopPropagation=true) Instead of adding arguments with default values, let's do "options" dictionary so that the call site would look like this: createEventHandler(() => { ~}, {preventDefault: true}) > Websites/perf.webkit.org/public/v3/components/base.js:257 > + if (preventDefault) > + event.preventDefault(); Then, here, you can do something like !('preventDefault' in options) || options['preventDefault'] > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:68 > + this.content().addEventListener('click', () => this._clearIndicatorState(false)); Just check event.target to be not a dependent of tooltip. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:70 > + this.content('tooltip-table').onclick = this.createEventHandler(() => {}, false, true); Instead of stopping the propagation here. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:77 > + }, false, false); I think we do we want to prevent default action in this case. We shouldn't let the browser do whatever it would normally do in this case. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:91 > + if (event.code == 'ArrowUp') { Use switch? > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:98 > + this._findAndFocusOnClosestIndicatorCell(column - 1, row, this._indicatorCellGrid, 'left-only'); Why don't we prevent default for left & right? Those would coming into play if the window size is small enough to warrant horizontal scrolling. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:101 > + }); It seems like we should just set closestIndicatorCell in each switch statement and then have a common patch to call focus() here. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:104 > + _findAndFocusOnClosestIndicatorCell(columnIndex, rowIndex, grid, direction) Instead of having this extra helper function, which is adding more layers of indirections. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:127 > + if (leftIndex >= 0 && row[leftIndex] && direction != 'right-only') We usually use camelCase so 'rightOnly' > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:130 > + if (rightIndex < row.length && row[rightIndex] && direction != 'left-only') and 'leftOnly' > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:297 > + platforms.forEach((platform, rowIndex) => { Why not just do this at this point? const tableBodyElement = platforms.map((~) => ~) > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:339 > + indicatorByMetric.set(metric, indicator); > + coordinateForIndicator.set(indicator, [rowIndex, columnIndex]); > + tabIndexForIndicator.set(indicator, tabIndex); > + indicatorCellsInCurrentRow.push(cell); All the arguments set into this map are coming from the caller. Why don't we just set these values in _renderTable instead? That'll make the code easier to follow. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:364 > + }, false, false); These false, false boolean arguments are problematic. Casual code reader can't tell what those false are specifying. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:370 > + }, true, true); Since these are true by default, there is no point in specifying that.
I think it's getting better but we probably need one more iteration of review.
Created attachment 380596 [details] Patch
Comment on attachment 380596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380596&action=review > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:96 > + event.preventDefault(); This should probably happen before the first indicator is focused above. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:98 > + switch(event.code) > + { Nit: switch (event.code) { So a space between switch & ( and no line break between ) and {. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:317 > + const cell = element('td', {class: 'status-cell', tabindex: currentTabIndex}, indicator); > + this._configureContainerCellForIndicator(cell, indicator); Hm... why don't we make _constructTableCell return cell & indicator instead? as in: const [indicator, cell] = this._constructTableCell(~); > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:324 > + currentTabIndex += 1; Nit: ++currentTabIndex. > Websites/perf.webkit.org/public/v3/pages/test-freshness-page.js:367 > + if (event.code == 'Enter' || event.code == 'Escape') { > + event.preventDefault(); > + event.stopPropagation(); > + this._showTooltip = event.code == 'Enter' ? !this._showTooltip : false; > + this.enqueueToRender(); We really shouldn't be listening to enter key like this. Instead, wrap each cell's content into 'a' element and listen for click.
Landed in r251028.