DevTools: clicking in console messages should not jump to bottom (issue 2840663002 by luoe@chromium.org)

[lu...@chromium.org]

Reviewers: dgozman
CL: https://codereview.chromium.org/2840663002/

Message:
ptal


https://codereview.chromium.org/2840663002/diff/20001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(left):

https://codereview.chromium.org/2840663002/diff/20001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#oldcode313
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:313:
// by focus().
This is no longer the case. Calling moveCaretToEndOfPrompt() will
always try to scroll.

https://codereview.chromium.org/2840663002/diff/20001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2840663002/diff/20001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#newcode118
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:118:
this._messagesElement.tabIndex = -1;
Currently on ToT, using up/down/pageup/pagedown keys in the console
viewport no longer scrolls. By doing this, the viewport is focusable,
restoring the old behavior. I haven't done a bisect yet to find out
when it broke.

https://codereview.chromium.org/2840663002/diff/20001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#newcode669
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:669:
this.focus();
I think we still need to keep 2 paths. When you click on the whitespace
underneath the prompt, it should still move the prompt cursor to the
end, which is the behavior on stable.

Description:
DevTools: clicking in console messages should not jump to bottom

ConsolePrompt's moveCaretToEndOfPrompt() works by setting the CodeMirror
editor's selection to the end. After a recent CodeMirror roll, doing so now
scrolls the editor into view.

This change in behavior led to an error where expanding an object in console
would jump the viewport to bottom. This CL only focuses the prompt without
moving the cursor, in some cases.

BUG=706128

Affected files (+119, -3 lines):
A third_party/WebKit/LayoutTests/inspector/console/console-focus.html
A third_party/WebKit/LayoutTests/inspector/console/console-focus-expected.txt
M third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js

[lu...@chromium.org]

A few more tests added and a fix to the case: clicking when scrolled to the
bottom with an existing selection.

Ptal

https://codereview.chromium.org/2840663002/

[pfel...@chromium.org]

https://codereview.chromium.org/2840663002/diff/60001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2840663002/diff/60001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#newcode312
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:312:
this._prompt.moveCaretToEndOfPrompt();
We probably should no longer call moveCaretToEndOfPrompt since we are no
longer trying to preserve scroll position. However, this does not fix
the regression when switching from Console to another tab and back was
losing focus.

https://codereview.chromium.org/2840663002/diff/60001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#newcode667
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:667:
if (!targetElement ||
!targetElement.enclosingNodeOrSelfWithClass('console-group'))
I think !targetElement.enclosingNodeOrSelfWithClass('console-group') is
always false, when is it not the case?

https://codereview.chromium.org/2840663002/

[lu...@chromium.org]

I've basically changed my CL to look like yours, except keeping the
moveCaretToEnd. My bad for not looking into the old tab switching regression.

Another test has been added, should cover all the interesting cases.

Please take a look.

https://codereview.chromium.org/2840663002/diff/60001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2840663002/diff/60001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#newcode312
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:312:
this._prompt.moveCaretToEndOfPrompt();

On 2017/04/25 07:29:30, pfeldman wrote:
> We probably should no longer call moveCaretToEndOfPrompt since we are
no longer
> trying to preserve scroll position. However, this does not fix the
regression
> when switching from Console to another tab and back was losing focus.

You're right, this patch reintroduces the old regression. I didn't
realize this, so I added a test in the latest patch set. In this case,
your CL's approach of restoring scrollTop sounds good to me.

If possible, I would still like to keep moveCaretToEndOfPrompt, however.
Not just for maintaining current behavior, but I find it very useful
when clicking below the prompt.

https://codereview.chromium.org/2840663002/diff/60001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#newcode667
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:667:
if (!targetElement ||
!targetElement.enclosingNodeOrSelfWithClass('console-group'))

On 2017/04/25 07:29:30, pfeldman wrote:
> I think !targetElement.enclosingNodeOrSelfWithClass('console-group')
is always
> false, when is it not the case?

When there are only a few messages, clicking in the area below the
prompt targets the viewport's element, which is a parent of the
'console-group' messages. In this case, the caret moves to the end.

https://codereview.chromium.org/2840663002/

[pfel...@chromium.org]

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#newcode314
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:314:
this._prompt.moveCaretToEndOfPrompt();

>> If possible, I would still like to keep moveCaretToEndOfPrompt,
however. Not just for maintaining current behavior, but I find it very
useful when clicking
below the prompt.

Could you elaborate on this? Is there a reason you would like toggling
between the tabs to reset your console selection?

https://codereview.chromium.org/2840663002/

[pfel...@chromium.org]

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html
File third_party/WebKit/LayoutTests/inspector/console/console-focus.html
(right):

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode22
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:22:
if (prompt._editor)
We should not have races in the tests, is there a reason to warrant that
the editor is there?

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode28
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:28:
InspectorTest.waitForConsoleMessages(22, beginTests);
() => InspectorTest.runTestSuite(testSuite)

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode35
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:35:
focusedElement.blur();
Where does this put the focus though? It might be better to focus
another element explicitly.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode45
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:45:
viewport.invalidate();
Changes to viewport API will now require updating this change, at the
same time the behavior you test has nothing to do with the viewport.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode46
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:46:
viewport.forceScrollItemToBeFirst(0);
ditto

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode49
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:49:
InspectorTest.addResult(String.sprintf("Test cannot be run as viewport
is not tall enough. It is required to contain at least %d messages, but
%d only fit", minimumViewportMessagesCount, viewportMessagesCount));
You can't have test giving up, it either works or does not, it can't say
"i can't run".

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode61
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:61:
dumpFocusInfo();
We should only dump the scrollTop, that's what we are testing. And we
should make sure it does not change, so we need to compare it to the
value _before_.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode69
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:69:
InspectorTest.addSniffer(ObjectUI.ObjectPropertyTreeElement,
"populateWithProperties", onExpanded);
Once again, we are not testing ObjectPropertySection here, so just one
test should suffice.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode102
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:102:
InspectorTest.evaluateInConsole("'history entry 1'", onMessageAdded);
Not sure what these are testing...

https://codereview.chromium.org/2840663002/

[pfel...@chromium.org]

https://codereview.chromium.org/2840663002/diff/120001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2840663002/diff/120001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#newcode311
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:311:
this._prompt.focus();
I think we still reset scroll position here, not?

https://codereview.chromium.org/2840663002/diff/120001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#newcode670
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:670:
this._prompt.moveCaretToEndOfPrompt();
Why moving caret to end of prompt is necessary?

https://codereview.chromium.org/2840663002/

[lu...@chromium.org]

Comments addressed, narrower test, moving caret to end only when the click
target was the container itself.

Please take another look.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html
File third_party/WebKit/LayoutTests/inspector/console/console-focus.html
(right):

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode22
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:22:
if (prompt._editor)

On 2017/04/25 18:40:56, pfeldman wrote:
> We should not have races in the tests, is there a reason to warrant
that the
> editor is there?

I need the editor for checking the cursor position. We can make it
cleaner with:
InspectorTest.waitUntilConsoleEditorLoaded()

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode28
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:28:
InspectorTest.waitForConsoleMessages(22, beginTests);

On 2017/04/25 18:40:56, pfeldman wrote:
> () => InspectorTest.runTestSuite(testSuite)

Done.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode35
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:35:
focusedElement.blur();

On 2017/04/25 18:40:56, pfeldman wrote:
> Where does this put the focus though? It might be better to focus
another
> element explicitly.

Done, document.body.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode45
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:45:
viewport.invalidate();

On 2017/04/25 18:40:56, pfeldman wrote:
> Changes to viewport API will now require updating this change, at the
same time
> the behavior you test has nothing to do with the viewport.

Removed.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode46
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:46:
viewport.forceScrollItemToBeFirst(0);

On 2017/04/25 18:40:56, pfeldman wrote:
> ditto

Replaced with setting scrollTop = 0.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode49
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:49:
InspectorTest.addResult(String.sprintf("Test cannot be run as viewport
is not tall enough. It is required to contain at least %d messages, but
%d only fit", minimumViewportMessagesCount, viewportMessagesCount));

On 2017/04/25 18:40:56, pfeldman wrote:
> You can't have test giving up, it either works or does not, it can't
say "i
> can't run".

Removed, this check doesn't really belong in a test about focus.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode61
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:61:
dumpFocusInfo();

On 2017/04/25 18:40:56, pfeldman wrote:
> We should only dump the scrollTop, that's what we are testing. And we
should
> make sure it does not change, so we need to compare it to the value
_before_.

Done.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode69
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:69:
InspectorTest.addSniffer(ObjectUI.ObjectPropertyTreeElement,
"populateWithProperties", onExpanded);

On 2017/04/25 18:40:56, pfeldman wrote:
> Once again, we are not testing ObjectPropertySection here, so just one
test
> should suffice.

Done. For some reason I thought this was only happening on object
expansion.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode102
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:102:
InspectorTest.evaluateInConsole("'history entry 1'", onMessageAdded);

On 2017/04/25 18:40:56, pfeldman wrote:
> Not sure what these are testing...

Removed.

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#newcode314
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:314:
this._prompt.moveCaretToEndOfPrompt();

On 2017/04/25 18:26:08, pfeldman wrote:
> >> If possible, I would still like to keep moveCaretToEndOfPrompt,
however. Not
> just for maintaining current behavior, but I find it very useful when
clicking
> below the prompt.
>
> Could you elaborate on this? Is there a reason you would like toggling
between
> the tabs to reset your console selection?

No, I really want the caret to move on messagesClicked when I click
underneath the prompt, not on focus/toggling between tabs. I find it
convenient when the console moves my cursor for me after I click.

https://codereview.chromium.org/2840663002/diff/120001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2840663002/diff/120001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#newcode311
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:311:
this._prompt.focus();

On 2017/04/25 18:50:17, pfeldman wrote:
> I think we still reset scroll position here, not?

No, only the moveCaretToEndOfPrompt did.

https://codereview.chromium.org/2840663002/diff/120001/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js#newcode670
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:670:
this._prompt.moveCaretToEndOfPrompt();

On 2017/04/25 18:50:17, pfeldman wrote:
> Why moving caret to end of prompt is necessary?

Please see above.

https://codereview.chromium.org/2840663002/

[pfel...@chromium.org]

https://codereview.chromium.org/2840663002/diff/140001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html
File third_party/WebKit/LayoutTests/inspector/console/console-focus.html
(right):

https://codereview.chromium.org/2840663002/diff/140001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode30
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:30:
function testClickingWithSelectedTextShouldNotFocusPrompt(next) {
But does it scroll? We are fixing scrolling here, so we should account
for it in the tests.

https://codereview.chromium.org/2840663002/diff/140001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode57
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:57:
function
testClickOutsideMessageListShouldFocusPromptAndMoveCaretToEnd(next) {
ditto

https://codereview.chromium.org/2840663002/

[lu...@chromium.org]

Both scrollTop and focus are now tested for!

https://codereview.chromium.org/2840663002/diff/140001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html
File third_party/WebKit/LayoutTests/inspector/console/console-focus.html
(right):

https://codereview.chromium.org/2840663002/diff/140001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode30
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:30:
function testClickingWithSelectedTextShouldNotFocusPrompt(next) {

On 2017/04/27 21:51:51, pfeldman wrote:
> But does it scroll? We are fixing scrolling here, so we should account
for it in
> the tests.

Done.

https://codereview.chromium.org/2840663002/diff/140001/third_party/WebKit/LayoutTests/inspector/console/console-focus.html#newcode57
third_party/WebKit/LayoutTests/inspector/console/console-focus.html:57:
function
testClickOutsideMessageListShouldFocusPromptAndMoveCaretToEnd(next) {

On 2017/04/27 21:51:51, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/2840663002/

[pfel...@chromium.org]

lgtm



https://codereview.chromium.org/2840663002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2840663002/160001

https://codereview.chromium.org/2840663002/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:
mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/440840)

https://codereview.chromium.org/2840663002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2840663002/180001

https://codereview.chromium.org/2840663002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/c5213f9bd6b6e6e95f40852b56ef62f1307c94ee

https://codereview.chromium.org/2840663002/