Bug 75901 – Improve keyboard navigation in layout test results

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

results.html needs some key handling to make test navigation more usable. I plan to add:

i/j/k/l for navigating the list
e/c/t for expand/collapse/toggle current test
some key to "flag" the current test
some way to copy out flagged tests

[Ojan Vafai]

Sounds great!

[Simon Fraser (smfr)]

Created attachment 121777 [details]
Patch

[WebKit Review Bot]

Comment on attachment 121777 [details]
Patch

Attachment 121777 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11183730

New failing tests:
fast/harness/results.html

[Ojan Vafai]

Comment on attachment 121777 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121777&action=review

This looks great. I have a bunch of cleanup nits. Feel free to not do any that you disagree with.

> LayoutTests/fast/harness/results.html:190
> +#flagged-tests {
> +    padding: 5px;
> +}

Should this be position:fixed to the bottom of the window? I can't decide if that would be nice or annoying. :)

> LayoutTests/fast/harness/results.html:942
> +TestNavigator.scrollToFirstTest = function()

Can you make all the properties/methods on TestNavigator except handleKeyEvent "private" (i.e. prefix with an underscore)?

> LayoutTests/fast/harness/results.html:945
> +    if (this.setCurrentTest(0))
> +        this.scrollToCurrentTest();

Here and below, I'd s/this/TestNavigator. While this code does work correctly, I find it confusing when "this" doesn't point to an instance of a class (i.e. something that was new'ed).

This way, TestNavigator essentially just acts as a namespace.

> LayoutTests/fast/harness/results.html:970
> +    var links = visibleExpandLinks();
> +    return links[this.currentTestIndex];

You could implement this as "return document.querySelector('.current .expand-button-text')".

> LayoutTests/fast/harness/results.html:1014
> +        var label = document.createElement('h2');
> +        label.innerText = 'Flagged Tests';
> +        flaggedTestContainer.appendChild(label);
> +        
> +        flaggedTestTextbox = document.createElement('div');
> +        flaggedTestTextbox.id = 'flagged-tests';
> +        flaggedTestTextbox.setAttribute('contentEditable', '');
> +        
> +        flaggedTestContainer.appendChild(flaggedTestTextbox);

It's your call, but I would prefer this to just use innerHTML since it's more concise and readable:
flaggedTestContainer.innerHTML = '<h2>Flagged Tests</h2><div id="flagged-tests" contentEditable></div>';
flaggedTestTextbox = document.getElementById('flagged-tests');

> LayoutTests/fast/harness/results.html:1020
> +    var flaggedTests = [];
> +    for (var test in this.flaggedTests)
> +        flaggedTests.push(test);

var flaggedTests = Object.keys(this.flaggedTests);

> LayoutTests/fast/harness/results.html:1026
> +TestNavigator.setCurrentTest = function(testIndex)

May as well call scrollToCurrentTest at the end of setCurrentTest since you always call it if you return true. Also, then setCurrentTest doesn't need a return value.

> LayoutTests/fast/harness/results.html:1039
> +    var currExpandButton = links[this.currentTestIndex];
> +    if (currExpandButton)
> +        currExpandButton.parentNode.classList.remove('current');
> +
> +    this.currentTestIndex = testIndex;
> +
> +    var currExpandButton = links[this.currentTestIndex];
> +    currExpandButton.parentNode.classList.add('current');

Nit: s/currExpandButton/currExpandLink/

Feel free to ignore this, but instead of maintaining currentTestIndex manually, you could rely on the fact that you're already adding/removing the appropriate class.

So, this could be implemented as:
var currExpandButton = document.querySelector('.current');
if (currExpandButton)
    currExpandButton.classList.remove('current');
links[testIndex].parentNode.classList.add('current');

The only place you really need currentTestIndex is when you're moving to the next/previous test. In that case, you can retrieve the index as:
function currentTestIndex() {
    var current = document.querySelector('.current');
    return Array.prototype.indexOf.call(visibleExpandLinks(), current);
}

> LayoutTests/fast/harness/results.html:1046
> +    var failedResultsTable = document.getElementById('results-table');

Dead code.

> LayoutTests/fast/harness/results.html:1047
> +    // Use visibleExpandLinks because it's already smart about expected failures.

In updateExpectedFailures(), we should probably just clear "current" if appropriate. Shouldn't be too hard:

if (onlyShowUnexpectedFailures() && parentOfType(TestNavigator.currentTestLink(), 'tbody').classList.contains('expected'))
  TestNavigator.currentTestLink().classList.remove('current');

> LayoutTests/fast/harness/results.html:1053
> +    var links = visibleExpandLinks();
> +    var testIndex = this.currentTestIndex;
> +    if (testIndex < 0 || testIndex >= links.length)
> +        return;
> +    
> +    var targetLink = links[testIndex];

You could implement this as:
var targetLink = this.currentTestLink();
if (!targetLink)
    return;

[Simon Fraser (smfr)]

(In reply to comment #4)
> (From update of attachment 121777 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121777&action=review
> 
> This looks great. I have a bunch of cleanup nits. Feel free to not do any that you disagree with.
> 
> > LayoutTests/fast/harness/results.html:190
> > +#flagged-tests {
> > +    padding: 5px;
> > +}
> 
> Should this be position:fixed to the bottom of the window? I can't decide if that would be nice or annoying. :)

Super annoying, it can get big.

> > LayoutTests/fast/harness/results.html:942
> > +TestNavigator.scrollToFirstTest = function()
> 
> Can you make all the properties/methods on TestNavigator except handleKeyEvent "private" (i.e. prefix with an underscore)?

Done.

> > LayoutTests/fast/harness/results.html:945
> > +    if (this.setCurrentTest(0))
> > +        this.scrollToCurrentTest();
> 
> Here and below, I'd s/this/TestNavigator. While this code does work correctly, I find it confusing when "this" doesn't point to an instance of a class (i.e. something that was new'ed).
> 
> This way, TestNavigator essentially just acts as a namespace.

OK. Not really keen on this because if we ever want > 1 TestNavigator I'd have to change it back. That seems unlikely though.

> > LayoutTests/fast/harness/results.html:970
> > +    var links = visibleExpandLinks();
> > +    return links[this.currentTestIndex];
> 
> You could implement this as "return document.querySelector('.current .expand-button-text')".

I could, but I'd rather use the same source of links everywhere.

> > LayoutTests/fast/harness/results.html:1014
> > +        var label = document.createElement('h2');
> > +        label.innerText = 'Flagged Tests';
> > +        flaggedTestContainer.appendChild(label);
> > +        
> > +        flaggedTestTextbox = document.createElement('div');
> > +        flaggedTestTextbox.id = 'flagged-tests';
> > +        flaggedTestTextbox.setAttribute('contentEditable', '');
> > +        
> > +        flaggedTestContainer.appendChild(flaggedTestTextbox);
> 
> It's your call, but I would prefer this to just use innerHTML since it's more concise and readable:
> flaggedTestContainer.innerHTML = '<h2>Flagged Tests</h2><div id="flagged-tests" contentEditable></div>';
> flaggedTestTextbox = document.getElementById('flagged-tests');

Done.

> > LayoutTests/fast/harness/results.html:1020
> > +    var flaggedTests = [];
> > +    for (var test in this.flaggedTests)
> > +        flaggedTests.push(test);
> 
> var flaggedTests = Object.keys(this.flaggedTests);

I was looking for a way to do that!

> > LayoutTests/fast/harness/results.html:1026
> > +TestNavigator.setCurrentTest = function(testIndex)
> 
> May as well call scrollToCurrentTest at the end of setCurrentTest since you always call it if you return true. Also, then setCurrentTest doesn't need a return value.

I want to keep them separate.

> > LayoutTests/fast/harness/results.html:1039
> > +    var currExpandButton = links[this.currentTestIndex];
> > +    if (currExpandButton)
> > +        currExpandButton.parentNode.classList.remove('current');
> > +
> > +    this.currentTestIndex = testIndex;
> > +
> > +    var currExpandButton = links[this.currentTestIndex];
> > +    currExpandButton.parentNode.classList.add('current');
> 
> Nit: s/currExpandButton/currExpandLink/
> 
> Feel free to ignore this, but instead of maintaining currentTestIndex manually, you could rely on the fact that you're already adding/removing the appropriate class.
> 
> So, this could be implemented as:
> var currExpandButton = document.querySelector('.current');
> if (currExpandButton)
>     currExpandButton.classList.remove('current');
> links[testIndex].parentNode.classList.add('current');
> 
> The only place you really need currentTestIndex is when you're moving to the next/previous test. In that case, you can retrieve the index as:
> function currentTestIndex() {
>     var current = document.querySelector('.current');
>     return Array.prototype.indexOf.call(visibleExpandLinks(), current);
> }

I didn't want to run a querySelector() every time I access currentTestIndex.

> > LayoutTests/fast/harness/results.html:1046
> > +    var failedResultsTable = document.getElementById('results-table');
> 
> Dead code.

Removed.

> > LayoutTests/fast/harness/results.html:1047
> > +    // Use visibleExpandLinks because it's already smart about expected failures.
> 
> In updateExpectedFailures(), we should probably just clear "current" if appropriate. Shouldn't be too hard:
> 
> if (onlyShowUnexpectedFailures() && parentOfType(TestNavigator.currentTestLink(), 'tbody').classList.contains('expected'))
>   TestNavigator.currentTestLink().classList.remove('current');

Holy encapsulation violation batman! I fixed this a cleaner way.

> > LayoutTests/fast/harness/results.html:1053
> > +    var links = visibleExpandLinks();
> > +    var testIndex = this.currentTestIndex;
> > +    if (testIndex < 0 || testIndex >= links.length)
> > +        return;
> > +    
> > +    var targetLink = links[testIndex];
> 
> You could implement this as:
> var targetLink = this.currentTestLink();
> if (!targetLink)
>     return;

Fixed.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/104606

[Ojan Vafai]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 121777 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=121777&action=review
> > 
> > > LayoutTests/fast/harness/results.html:1047
> > > +    // Use visibleExpandLinks because it's already smart about expected failures.
> > 
> > In updateExpectedFailures(), we should probably just clear "current" if appropriate. Shouldn't be too hard:
> > 
> > if (onlyShowUnexpectedFailures() && parentOfType(TestNavigator.currentTestLink(), 'tbody').classList.contains('expected'))
> >   TestNavigator.currentTestLink().classList.remove('current');
> 
> Holy encapsulation violation batman! I fixed this a cleaner way.

Lol. Yeah, I was a bit rushed as I was finishing this review. FYI, you accidentally left in this code commented out in the final checkin.

Thanks for adding all this! I'm eager to try it out.

[Ojan Vafai]

Also, this just occurred to me, should we add a legend or something so this is a bit more discoverable? I already find myself forgetting the key commands each time.

[Simon Fraser (smfr)]

There is a legend.