[webkit-reviews] review granted: [Bug 202684] Improve test freshness page interaction experience. : [Attachment 380596] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 202684: Improve test freshness page interaction experience.
https://bugs.webkit.org/show_bug.cgi?id=202684

Attachment 380596: Patch

https://bugs.webkit.org/attachment.cgi?id=380596&action=review




--- Comment #9 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 380596
  --> https://bugs.webkit.org/attachment.cgi?id=380596
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.