Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2179123004:
DevTools: fix stick to bottom in console viewport (Closed)
|
Created:
4 years, 4 months ago by luoe Modified:
4 years, 4 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, sergeyv+blink_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: fix stick to bottom in console viewport
BUG=603294
Committed: https://crrev.com/47da3b4f70636bbac1d874a6710ee6a5806b5392
Cr-Commit-Position: refs/heads/master@{#411829}
Patch Set 1 #Patch Set 2 : Best attempt at tests #
Total comments: 8
Patch Set 3 : Address comments #
Total comments: 18
Patch Set 4 : Address comments #
Total comments: 2
Patch Set 5 : Rename forTest fn() #
Total comments: 12
Patch Set 6 : rebase #Patch Set 7 : Address comments #
Total comments: 12
Patch Set 8 : Address comments #Patch Set 9 : Rename ...onMouseWheel to onWheel #Patch Set 10 : Comments addressed. #
Total comments: 8
Patch Set 11 : Introduce partialUpdate flag, address comments #Patch Set 12 : rebase #Patch Set 13 : Replace partial flag with 'maybeDirtyWhileMuted' #Patch Set 14 : Don't stick onkeydown when prompt fills viewport #
Total comments: 8
Patch Set 15 : Address comments #Patch Set 16 : Comments addressed #
Total comments: 1
Patch Set 17 : Add check for viewport range changes #Patch Set 18 : Visible ranges too #
Total comments: 1
Patch Set 19 : Remove all range logic, pageUp is no longer special #
Messages
Total messages: 40 (12 generated)
luoe@chromium.org changed reviewers: + dgozman@chromium.org, lushnikov@chromium.org
This was built with a ton of <3 from Pavel! Note: Patch 2 includes a commented out attempt at writing tests. Since the scroll logic now includes a setTimeout with a hard-coded duration, I couldn't figure out a great way to write tests.
https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js (right): https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js:63: this._observer = new MutationObserver(this.refresh.bind(this)); what do you use mutation observer for? Could you please add a comment for this?
To overcome hardcoded timeout we can hardcode larger timeout in the test itself! https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1033: setTimeout(updateViewportState.bind(this), 200); I'm worried this timeout is not compatible with fast mouse clicks/drags. https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1069: if (isEsc) Why this? https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js (right): https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js:66: this.setStickToBottom(true); It was false by default before.
Comments addressed and tests updated. Ptal https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1033: setTimeout(updateViewportState.bind(this), 200); On 2016/07/30 16:15:54, dgozman wrote: > I'm worried this timeout is not compatible with fast mouse clicks/drags. A hardcoded value in a timeout worries me too, but I think this is a good approach. Bad cases are when the user (1) wants to stick but cannot, or if they (2) want to un-stick but remain stuck. Case 1: If the user wants to stick, that means that on mousedown, the bar starts out above the bottom, so when they are done interacting, the onmouseup code will check isScrolledToBottom immediately (won't trigger a setTimeout). It is possible to end up in a bad case only if a previously scheduled setTimeout calls updateViewportState when we don't expect. I can easily reproduce this bad behavior with: - start@bottom > Click > keydown immediately (<200ms) after click Case 2: If the user wants to un-stick, the only way it would stay stuck is if we think the scroll up has occurred but no scroll events have been processed yet. This should only be possible in rare cases if smooth scrolling took longer than 200ms. Since case 2 *should never happen, I think having bad case 1 is acceptable. Clicking and pressing a key within ~200ms is easy to repro, but probably isn't too common. If they really intend to stick to the bottom, there will likely be a sequence of keypresses that take >200ms to type. In the worst case, users can always guarantee stick/unstick to bottom by waiting 200ms in between their actions. https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1069: if (isEsc) On 2016/07/30 16:15:53, dgozman wrote: > Why this? Any keydown can set stick to bottom == true. I believe Escape is a special case because it always toggles the Drawer, which shouldn't signal a jump to the bottom. https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js (right): https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js:63: this._observer = new MutationObserver(this.refresh.bind(this)); On 2016/07/30 01:31:12, lushnikov wrote: > what do you use mutation observer for? Could you please > add a comment for this? MutationObserver ensures that a refresh will be fired if an item in the list updates, which may change the scroll height. Comment added. https://codereview.chromium.org/2179123004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js:66: this.setStickToBottom(true); On 2016/07/30 16:15:54, dgozman wrote: > It was false by default before. Right, I'll remove this.
https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html (right): https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:82: stray line https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:84: function testViewportMuationsShouldPreserveStickToBottom(next) Muations -> Mutations https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:97: eventSender.keyDown('Tab'); let's not use eventSender if possible https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:102: function refreshLogAndContinue(callback) in multiple tests across inspctor, we call such methods "dump...". Let's call this dumpAndContinue https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1025: { if (!this._muteViewportUpdates) return; So that we don't run anything for innocent mouseleave events. https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1027: if (this._wasScrolledToBottom) { instead of having this logic, can we have a passive scroll listener which updates the stickToBottom? This will eliminate the timeout https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1051: if (fromTimeout) can we call this unconditionally? https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1071: if (isEsc) let's bail out in the very beginning of the function https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js (right): https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js:427: // text prompt, we may overestimate the number of next visible indices when new messages are mb underestimate, not overestimate?
Thanks for the detailed review. Comments addressed https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html (right): https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:82: On 2016/08/02 00:10:48, lushnikov wrote: > stray line Done. https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:84: function testViewportMuationsShouldPreserveStickToBottom(next) On 2016/08/02 00:10:47, lushnikov wrote: > Muations -> Mutations Done. https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:97: eventSender.keyDown('Tab'); On 2016/08/02 00:10:47, lushnikov wrote: > let's not use eventSender if possible Done. https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:102: function refreshLogAndContinue(callback) On 2016/08/02 00:10:47, lushnikov wrote: > in multiple tests across inspctor, we call such methods "dump...". > > Let's call this dumpAndContinue Done. https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1025: { On 2016/08/02 00:10:48, lushnikov wrote: > if (!this._muteViewportUpdates) > return; > > So that we don't run anything for innocent mouseleave events. Done. https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1027: if (this._wasScrolledToBottom) { On 2016/08/02 00:10:48, lushnikov wrote: > instead of having this logic, can we have a passive scroll listener which > updates the stickToBottom? > This will eliminate the timeout I think it's still necessary to wait. Even when the listener is passive and scroll events aren't blocked, in a fast scrollbar click, the order of events is still mousedown > mouseup > scroll. When the mouseup fires updateViewportState() before any scroll events, it will make us stick to the bottom when the user wants to un-stick. https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1051: if (fromTimeout) On 2016/08/02 00:10:48, lushnikov wrote: > can we call this unconditionally? Done. https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1071: if (isEsc) On 2016/08/02 00:10:48, lushnikov wrote: > let's bail out in the very beginning of the function Done. https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js (right): https://codereview.chromium.org/2179123004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js:427: // text prompt, we may overestimate the number of next visible indices when new messages are On 2016/08/02 00:10:48, lushnikov wrote: > mb underestimate, not overestimate? I think this might depend on the situation. If we scroll up and away from the bottom, you're correct in that we underestimate. If more console messages are added to the bottom while we stick, the jump to the bottom we can overestimate. I'll change the comment.
this looks good for me. The 200ms timeout (and the necessity of waiting for it in test) is the only unappealing part, but I don't have a better idea. lgtm. Please, make check with @dgozman if he has ideas about 200ms https://codereview.chromium.org/2179123004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1056: _updateViewportForTest: function() _updateViewportStickinessForTest
Function renamed. Ptal https://codereview.chromium.org/2179123004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1056: _updateViewportForTest: function() On 2016/08/02 19:37:25, lushnikov wrote: > _updateViewportStickinessForTest Done.
The CQ bit was checked by lushnikov@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...
https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html (right): https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:66: function onUpdateTimeout() { style: { on next line https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:87: viewport._contentElement.children[1].innerText = "More than 2 lines: foo\n\n\n\nbar"; Does this really make it multiline? Maybe use br instead? https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1067: _updateStickToBottomOnKeyDown: function(event) Can we instead listen to prompt text changed or something? https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1069: if (event.key === "Escape") This still looks really hacky. What happens if I do Ctrl+Shift+F? Or any other shortcut we have? https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1072: if (event.key === "PageUp" || event.key === "PageDown") Ctrl+Home/End? Opt+Something on mac? There could be more. https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js (right): https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js:67: this._observer = new MutationObserver(this.refresh.bind(this)); Did you check the performance of this? E.g. do console.table() with a lot of rows and measure.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Comments addressed, ptal dgozman@. Notes: - _updateStickToBottomOnMouseUp() didn't actually need to make another scheduleViewportUpdate() call, since the viewport's scroll handler should take care of that. - keys that navigate but do not trigger an 'input' change event are handled specially in _updateStickToBottomOnKeyDown(). Home/End/PageUp/PageDown/Arrows are all ones I thought should affect stickiness. https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html (right): https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:66: function onUpdateTimeout() { On 2016/08/03 21:19:00, dgozman wrote: > style: { on next line Done. https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:87: viewport._contentElement.children[1].innerText = "More than 2 lines: foo\n\n\n\nbar"; On 2016/08/03 21:19:00, dgozman wrote: > Does this really make it multiline? Maybe use br instead? Yes, it does make it multiline, the '\n' is auto-replaced with <br>. It's also used on line #9 as well. https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1067: _updateStickToBottomOnKeyDown: function(event) On 2016/08/03 21:19:01, dgozman wrote: > Can we instead listen to prompt text changed or something? Yeah, that would be better. https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1069: if (event.key === "Escape") On 2016/08/03 21:19:01, dgozman wrote: > This still looks really hacky. What happens if I do Ctrl+Shift+F? Or any other > shortcut we have? You're right, I hadn't thought of that. Modified to fire a stick to bottom on input events (includes paste). https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1072: if (event.key === "PageUp" || event.key === "PageDown") On 2016/08/03 21:19:00, dgozman wrote: > Ctrl+Home/End? Opt+Something on mac? There could be more. On Mac, Fn-Arrow keys map to PageUp/Down/Home/End. When pressing Fn-ArrowUp, keydown handler still fires just one PageUp event with key == "PageUp", even though two keys are pressed. So this should be handled correctly. Although I didn't take into account regular Home/End keys. Will do. https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js (right): https://codereview.chromium.org/2179123004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ViewportControl.js:67: this._observer = new MutationObserver(this.refresh.bind(this)); On 2016/08/03 21:19:01, dgozman wrote: > Did you check the performance of this? E.g. do console.table() with a lot of > rows and measure. Scrolling with a console.table() was slow! Not because of the mutation observer, but because there was an invalidate() and full viewport update called every scroll. I've removed the unnecessary scheduleViewportRefresh() in ConsoleView.js:1052 to address this. Mutation observer doesn't run its handler while a viewport update occurs, so it should be fine.
https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1062: _updateStickToBottomOnMouseWheel: function() Do we handle touchpad here? Are there any other types of input to handle? Could it be that scroll/input api already has a mechanism to handle this? https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1084: else if (event.key === "PageUp") PageUp/PageDown are handled by multiline prompt - it moves the caret. Can we avoid listening to the keyboard at all? As I understand it, we want to stick to bottom when prompt gets attention (either input or selection changed).
https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:163: this._messagesElement.addEventListener("keydown", this._updateStickToBottomOnKeyDown.bind(this), true); false https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1023: this._wasScrolledToBottom = this._messagesElement.isScrolledToBottom(); This we can remove. https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1031: this._muteViewportUpdates = false; This goes to updateViewportState, together with scheduleUpdate. https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1085: this._updateStickToBottomOnMouseWheel(); We call this for all the keys listed above.
comments addressed. https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:163: this._messagesElement.addEventListener("keydown", this._updateStickToBottomOnKeyDown.bind(this), true); On 2016/08/05 17:36:42, dgozman wrote: > false Done. https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1023: this._wasScrolledToBottom = this._messagesElement.isScrolledToBottom(); On 2016/08/05 17:36:41, dgozman wrote: > This we can remove. Done. https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1031: this._muteViewportUpdates = false; On 2016/08/05 17:36:41, dgozman wrote: > This goes to updateViewportState, together with scheduleUpdate. Done. https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1062: _updateStickToBottomOnMouseWheel: function() On 2016/08/05 17:10:01, dgozman wrote: > Do we handle touchpad here? Are there any other types of input to handle? > Could it be that scroll/input api already has a mechanism to handle this? Good catch, switched from 'mousewheel' to 'wheel'. Tested on Mac, touchpad scrolls & flings fire wheel event. According to MDN, 'wheel' can be generated by an actual mouse or touch event. https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1084: else if (event.key === "PageUp") On 2016/08/05 17:10:01, dgozman wrote: > PageUp/PageDown are handled by multiline prompt - it moves the caret. > Can we avoid listening to the keyboard at all? As I understand it, we want to > stick to bottom when prompt gets attention (either input or selection changed). We can actually move this logic into promptKeyDown(), which should fire when the prompt is focused. https://codereview.chromium.org/2179123004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1085: this._updateStickToBottomOnMouseWheel(); On 2016/08/05 17:36:41, dgozman wrote: > We call this for all the keys listed above. After manual testing, it looks like the rest of these don't need to call updateStickToBottomOnMouseWheel. Native behavior handles them fine
https://codereview.chromium.org/2179123004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:767: if (event.key === "PageUp") { Let's move this back to keydown on element. https://codereview.chromium.org/2179123004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1034: setTimeout(updateViewportState.bind(this), 200); Let's cancel previous one if any. https://codereview.chromium.org/2179123004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1041: this._muteViewportUpdates = false; We should schedule update here. https://codereview.chromium.org/2179123004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1060: this._viewport.setStickToBottom(true); Let's use _immediatelyScrollToBottom.
Noticeable changes: - addition of a boolean partialUpdateRefresh flag to fix a slow case with console.table - leaving the keydown logic inside promptKeyDown's handler https://codereview.chromium.org/2179123004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:767: if (event.key === "PageUp") { On 2016/08/05 21:08:25, dgozman wrote: > Let's move this back to keydown on element. On a closer look, I think this actually can stay. The reason why we decided on moving it back was because we want to run the handler when the prompt is not focused. However, if the prompt is not focused, the consoleView isn't focused either and the listener won't fire. If we put a keydown listener on the entire document, it seems that native PageUp scrolling only happens when the selected text is 'in view'. Once it scrolls the selection out of view, it doesn't scroll. https://codereview.chromium.org/2179123004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1034: setTimeout(updateViewportState.bind(this), 200); On 2016/08/05 21:08:25, dgozman wrote: > Let's cancel previous one if any. Done. https://codereview.chromium.org/2179123004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1041: this._muteViewportUpdates = false; On 2016/08/05 21:08:25, dgozman wrote: > We should schedule update here. We should. Doing so introduces an unfortunate slowdown when scrolling with a console.table(document.all). The reason it's slow is because the scheduleViewportRefresh() does a ViewportControl.fullViewportUpdate(). Can we add a flag that tells it to run partial updates only? https://codereview.chromium.org/2179123004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1060: this._viewport.setStickToBottom(true); On 2016/08/05 21:08:25, dgozman wrote: > Let's use _immediatelyScrollToBottom. Done.
Removed partialRefresh flag. Introduced 'maybeDirtyWhileMuted' check so that we scheduleViewportUpdates when possibly dirty. Ptal
On keydown, if the prompt fills the entire viewport, we should not jump down. Ptal
https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:383: if (this._muteViewportUpdates) Let's do dirty flag here as well. https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1038: this._muteViewportUpdates = false; Also do |delete this._waitForScrollTimeout| https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1041: this._scheduleViewportRefresh(); Let's add a test which adds new messages while user is dragging (we can call these methods directly). https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1063: this._viewport.setStickToBottom(false); I don't think we should set it false - just return instead.
I'm not sure that I wrote the test in the best way possible. It currently has more sniffers than I'd like. Ptal. https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:383: if (this._muteViewportUpdates) On 2016/08/09 21:16:47, dgozman wrote: > Let's do dirty flag here as well. Done. https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1038: this._muteViewportUpdates = false; On 2016/08/09 21:16:47, dgozman wrote: > Also do |delete this._waitForScrollTimeout| Done. https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1041: this._scheduleViewportRefresh(); On 2016/08/09 21:16:47, dgozman wrote: > Let's add a test which adds new messages while user is dragging (we can call > these methods directly). Done. https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1063: this._viewport.setStickToBottom(false); On 2016/08/09 21:16:47, dgozman wrote: > I don't think we should set it false - just return instead. Done.
I'm not sure that I wrote the test in the best way possible. It currently has more sniffers than I'd like. Ptal. https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:383: if (this._muteViewportUpdates) On 2016/08/09 21:16:47, dgozman wrote: > Let's do dirty flag here as well. Done. https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1038: this._muteViewportUpdates = false; On 2016/08/09 21:16:47, dgozman wrote: > Also do |delete this._waitForScrollTimeout| Done. https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1041: this._scheduleViewportRefresh(); On 2016/08/09 21:16:47, dgozman wrote: > Let's add a test which adds new messages while user is dragging (we can call > these methods directly). Done. https://codereview.chromium.org/2179123004/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1063: this._viewport.setStickToBottom(false); On 2016/08/09 21:16:47, dgozman wrote: > I don't think we should set it false - just return instead. Done.
Just a single comment. Thanks for the tests! https://codereview.chromium.org/2179123004/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:773: if (event.key === "PageUp") { Why do we only handle PageUp, and only here? What about Home when user selects some text while stickToBottom == true?
Visible ranges logic added.... Ptal
Comments addressed, no more range logic!!! https://codereview.chromium.org/2179123004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js (right): https://codereview.chromium.org/2179123004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:773: if (event.key === "PageUp") { Remove this
lgtm
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 lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2179123004/#ps350001 (title: "Remove all range logic, pageUp is no longer special")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #19 (id:350001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: fix stick to bottom in console viewport BUG=603294 ========== to ========== DevTools: fix stick to bottom in console viewport BUG=603294 Committed: https://crrev.com/47da3b4f70636bbac1d874a6710ee6a5806b5392 Cr-Commit-Position: refs/heads/master@{#411829} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/47da3b4f70636bbac1d874a6710ee6a5806b5392 Cr-Commit-Position: refs/heads/master@{#411829}
Message was sent while issue was closed.
A revert of this CL (patchset #19 id:350001) has been created in https://codereview.chromium.org/2256873002/ by lushnikov@chromium.org. The reason for reverting is: Reverting as this adds 200 ms for evaluation commands in console. Repro: 1. open console 2. evaluate "1" 3. wait for response. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
