Bug 202684 – Improve test freshness page interaction experience.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Improve test freshness page interaction experience.

Created attachment 380405 [details]
Patch

Created attachment 380454 [details]
Patch

Created attachment 380455 [details]
Patch

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

I think it's getting better but we probably need one more iteration of review.

Created attachment 380596 [details]
Patch

[Ryosuke Niwa]

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.