Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2840663002:
DevTools: clicking in console messages should not jump to bottom (Closed)
|
Created:
3 years, 8 months ago by luoe Modified:
3 years, 7 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: 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
Review-Url: https://codereview.chromium.org/2840663002
Cr-Commit-Position: refs/heads/master@{#468009}
Committed: https://chromium.googlesource.com/chromium/src/+/c5213f9bd6b6e6e95f40852b56ef62f1307c94ee
Patch Set 1 #Patch Set 2 : test #
Total comments: 3
Patch Set 3 : fix clicking below prompt condition and add tests #Patch Set 4 : test #
Total comments: 4
Patch Set 5 : restore scroll pos, add test for old regression #
Total comments: 20
Patch Set 6 : move caret only on click, not focus #
Total comments: 4
Patch Set 7 : ac #
Total comments: 4
Patch Set 8 : ac #Patch Set 9 : fix platform dependent test #
Messages
Total messages: 30 (15 generated)
Description was changed from ========== DevTools: do not focus in console prompt when messages are clicked BUG=706128 ========== to ========== 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 ==========
luoe@chromium.org changed reviewers: + dgozman@chromium.org
ptal https://codereview.chromium.org/2840663002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (left): https://codereview.chromium.org/2840663002/diff/20001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2840663002/diff/20001/third_party/WebKit/Sour... 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/Sour... 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 was changed from ========== 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 ========== to ========== 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 ==========
luoe@chromium.org changed reviewers: + einbinder@chromium.org, pfeldman@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/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2840663002/diff/60001/third_party/WebKit/Sour... 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/Sour... 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?
Patchset #5 (id:80001) has been deleted
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/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2840663002/diff/60001/third_party/WebKit/Sour... 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/Sour... 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/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/Sou... 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/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/inspector/console/console-focus.html (right): https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/Lay... 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/Lay... 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/Lay... 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/Lay... 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/Lay... third_party/WebKit/LayoutTests/inspector/console/console-focus.html:46: viewport.forceScrollItemToBeFirst(0); ditto https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/Lay... 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/Lay... 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/Lay... 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/Lay... 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/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2840663002/diff/120001/third_party/WebKit/Sou... 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/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:670: this._prompt.moveCaretToEndOfPrompt(); Why moving caret to end of prompt is necessary?
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/Lay... File third_party/WebKit/LayoutTests/inspector/console/console-focus.html (right): https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/Lay... 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/Lay... 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/Lay... 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/Lay... 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/Lay... 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/Lay... 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/Lay... 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/Lay... 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/Lay... 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/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2840663002/diff/100001/third_party/WebKit/Sou... 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/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2840663002/diff/120001/third_party/WebKit/Sou... 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/Sou... 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/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/inspector/console/console-focus.html (right): https://codereview.chromium.org/2840663002/diff/140001/third_party/WebKit/Lay... 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/Lay... third_party/WebKit/LayoutTests/inspector/console/console-focus.html:57: function testClickOutsideMessageListShouldFocusPromptAndMoveCaretToEnd(next) { ditto
Both scrollTop and focus are now tested for! https://codereview.chromium.org/2840663002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/inspector/console/console-focus.html (right): https://codereview.chromium.org/2840663002/diff/140001/third_party/WebKit/Lay... 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/Lay... third_party/WebKit/LayoutTests/inspector/console/console-focus.html:57: function testClickOutsideMessageListShouldFocusPromptAndMoveCaretToEnd(next) { On 2017/04/27 21:51:51, pfeldman wrote: > ditto Done.
lgtm
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@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_...)
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2840663002/#ps180001 (title: "fix platform dependent test")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493393072353240,
"parent_rev": "37c9356daeb495268f0ccc950c2c8095c0edd0e9", "commit_rev":
"c5213f9bd6b6e6e95f40852b56ef62f1307c94ee"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Review-Url: https://codereview.chromium.org/2840663002 Cr-Commit-Position: refs/heads/master@{#468009} Committed: https://chromium.googlesource.com/chromium/src/+/c5213f9bd6b6e6e95f40852b56ef... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/c5213f9bd6b6e6e95f40852b56ef... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
