[webkit-reviews] review granted: [Bug 75901] Improve keyboard navigation in layout test results : [Attachment 121777] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 75901: Improve keyboard navigation in layout test results
https://bugs.webkit.org/show_bug.cgi?id=75901

Attachment 121777: Patch
https://bugs.webkit.org/attachment.cgi?id=121777&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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;