Bug 92584 – i and j keys no longer scroll in results.html

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

I added support for the i, j, k, l keys in results.html, and they should scroll to the selected result. The scrolling no longer works. I can hear the baby jesus crying.

[Simon Fraser (smfr)]

This broke in http://trac.webkit.org/changeset/122880 when the style was changed to body { display: -webkit-flex; }

[Ojan Vafai]

Created attachment 155631 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 155631 [details]
Patch

Why all this craziness, and not just a normal scrolling document?

[Tony Chang]

Comment on attachment 155631 [details]
Patch

Sorry, didn't see Simon's comment.

[Ojan Vafai]

(In reply to comment #3)
> (From update of attachment 155631 [details])
> Why all this craziness, and not just a normal scrolling document?

It's not actually that much craziness, is it? It's an extra container flexbox div and scrolling a div element instead of the window. The rest of the code changes are just fixing unrelated bugs I stumbled upon as I was testing this.

It's so the flagged tests always show up at the bottom of the viewport. I was watching people over their shoulders using flagging and they didn't understand what flagging did because the text box was scrolled out of view. Also, it bugs me when I flag things and I need to scroll to get the text box.

[Simon Fraser (smfr)]

Can't the flagging box just be position:fixed, bottom: 10px?

[Ojan Vafai]

(In reply to comment #6)
> Can't the flagging box just be position:fixed, bottom: 10px?

Then it overlaps the content at the end of the page.

[Ojan Vafai]

Comment on attachment 155631 [details]
Patch

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

> LayoutTests/fast/harness/results.html:1121
> +    var container = document.querySelector('.content-container');
>      // rowRect is in client coords (i.e. relative to viewport), so we just want to add its top to the current scroll position.
> -    window.scrollTo(window.scrollX, window.scrollY + rowRect.top - 20);
> +    container.scrollTop += rowRect.top - 20;

To be clear, this is the only part of this patch that is needed for fixing the scrolling. The rest is to make navigating/flagging unexpected passes work.

[Ojan Vafai]

Comment on attachment 155631 [details]
Patch

Marking cq+. Simon, it doesn't seem like you object to this getting committed. If you do, feel free to cq- or rollout if this get committed before you notice.

[Simon Fraser (smfr)]

I'm ok with it.

[WebKit Review Bot]

Comment on attachment 155631 [details]
Patch

Clearing flags on attachment: 155631

Committed r124287: <http://trac.webkit.org/changeset/124287>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.