Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2646283002:
ClassesPaneWidget - Add ability to quickly preview autocompleted CSS classes. (Closed)
|
Created:
3 years, 11 months ago by kdzwinel Modified:
3 years, 8 months ago Reviewers:
lushnikov CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: ClassesPaneWidget - Add ability to quickly preview autocompleted CSS classes.
In action - https://i.imgur.com/yucn4TI.gif 📽
BUG=683623
Review-Url: https://codereview.chromium.org/2646283002
Cr-Commit-Position: refs/heads/master@{#463104}
Committed: https://chromium.googlesource.com/chromium/src/+/56d5720cf9a277a61d859d6829bf1a5bfb7d845c
Patch Set 1 #Patch Set 2 : Code cleanup #Patch Set 3 : Add missing jsdoc #
Total comments: 8
Patch Set 4 : Rewrite based on @lushnikov's code #Patch Set 5 : Do not modify node if there are no classes being previewed. #Patch Set 6 : Revert FlavorChanged change listener. Prevent reace condition in _installNodeClasses. #
Total comments: 4
Patch Set 7 : Use Common.Throttler; Fix textprompt issue; Fix ESC key issue #
Total comments: 5
Patch Set 8 : Fixes after code review. #
Total comments: 4
Patch Set 9 : Code review fixes. #Patch Set 10 : Clean up Enter key handling. #
Messages
Total messages: 27 (8 generated)
Description was changed from ========== ClassesPaneWidget: Apply autocompleted CSS classes immediately. BUG= ========== to ========== ClassesPaneWidget: Apply autocompleted CSS classes immediately. ATM it's a proof of concept. Not ready for the review. BUG=683623 ==========
Description was changed from ========== ClassesPaneWidget: Apply autocompleted CSS classes immediately. ATM it's a proof of concept. Not ready for the review. BUG=683623 ========== to ========== ClassesPaneWidget: Apply autocompleted CSS classes immediately. Preview - https://i.imgur.com/yucn4TI.gif BUG=683623 ==========
kdzwinel@gmail.com changed reviewers: + lushnikov@chromium.org
Description was changed from ========== ClassesPaneWidget: Apply autocompleted CSS classes immediately. Preview - https://i.imgur.com/yucn4TI.gif BUG=683623 ========== to ========== ClassesPaneWidget: Add ability to quickly preview autocompleted CSS classes. In action - https://i.imgur.com/yucn4TI.gif 📽 BUG=683623 ==========
PTAL
Hey @kdzwinel, Thanks for pinging me in the bug and sorry for the wait! As I wrote the comments to the patch, I've done a prototype of a less-involved approach to implement the functionality: https://codereview.chromium.org/2682743003 wdyt? https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:22: this._previewClasses = new Set(); Why do we need a set? afaiu there's only one suggestion possible at a time https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:38: const text = event.target.textContent; nit: we don't use consts so far, let's use "var" https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:135: const classNames = text.split(/[.,\s]/); that's unfortunate we had to copy the regex over here https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:149: _getActiveClasses(node) { why do you need to extract this?
Thank you for a review! Your solution is much simpler but it might have two small issues: 1) it doesn't work well with multiple classes in the "Add new class" field (only the last one is applied) and 2) it doesn't remove the class being previewed if user aborts. See: https://i.imgur.com/PEn3Bh2.gif #2 can probably be easily fixed. #1 is debatable, I'd expect both classes to be previewed (because that's how the end result will look like if user clicks Enter), but maybe we don't really need that? If so, we should definitely go with your solution. Cheers, Konrad https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:22: this._previewClasses = new Set(); On 2017/02/07 at 22:01:37, lushnikov wrote: > Why do we need a set? afaiu there's only one suggestion possible at a time One suggestion, but there can be multiple classes in the 'Add new class' field at the time. https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:38: const text = event.target.textContent; On 2017/02/07 at 22:01:37, lushnikov wrote: > nit: we don't use consts so far, let's use "var" AFAIK `let` is not yet used, but `const` is fine. I used it in my previous CL ( https://codereview.chromium.org/2567873002 ). https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:135: const classNames = text.split(/[.,\s]/); On 2017/02/07 at 22:01:37, lushnikov wrote: > that's unfortunate we had to copy the regex over here Good point! It should probably be extracted to a constant or a `_splitIntoClasses` method. https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:149: _getActiveClasses(node) { On 2017/02/07 at 22:01:37, lushnikov wrote: > why do you need to extract this? No real need, I did it just for the readability of _installNodeClasses, but maybe it was unnecessary?
On 2017/02/07 23:20:06, kdzwinel wrote: > Thank you for a review! > > Your solution is much simpler but it might have two small issues: > 1) it doesn't work well with multiple classes in the "Add new class" field (only > the last one is applied) and > 2) it doesn't remove the class being previewed if user aborts. > > See: https://i.imgur.com/PEn3Bh2.gif Ah, I see now. I talked to Joel (who's our TextPrompt guru), and he came up with a patch which improves text prompt: https://codereview.chromium.org/2681043002/ As it is landed, it should be easy to address both (1) and (2). Hope this helps! > > #2 can probably be easily fixed. #1 is debatable, I'd expect both classes to be > previewed (because that's how the end result will look like if user clicks > Enter), but maybe we don't really need that? If so, we should definitely go with > your solution. > > Cheers, > Konrad > > https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js > (right): > > https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:22: > this._previewClasses = new Set(); > On 2017/02/07 at 22:01:37, lushnikov wrote: > > Why do we need a set? afaiu there's only one suggestion possible at a time > > One suggestion, but there can be multiple classes in the 'Add new class' field > at the time. > > https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:38: > const text = event.target.textContent; > On 2017/02/07 at 22:01:37, lushnikov wrote: > > nit: we don't use consts so far, let's use "var" > > AFAIK `let` is not yet used, but `const` is fine. I used it in my previous CL ( > https://codereview.chromium.org/2567873002 ). > > https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:135: > const classNames = text.split(/[.,\s]/); > On 2017/02/07 at 22:01:37, lushnikov wrote: > > that's unfortunate we had to copy the regex over here > > Good point! It should probably be extracted to a constant or a > `_splitIntoClasses` method. > > https://codereview.chromium.org/2646283002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:149: > _getActiveClasses(node) { > On 2017/02/07 at 22:01:37, lushnikov wrote: > > why do you need to extract this? > > No real need, I did it just for the readability of _installNodeClasses, but > maybe it was unnecessary?
It does help! Thank you. I adopted your code and fixed issues #1 and #2. Unfortunately, I run into two more problems: 3) https://crbug.com/690255 - this is minor and will be handled separately 4) previewed classes are not getting discarded when inspected node changes. See: https://i.imgur.com/GUz6ZTF.gif . Whenever inspected node changes, ClassesPaneWidget._update is called, however we no longer have access to the previously inspected node at this point. Maybe you'll have an idea how to get around this? Cheers, Konrad
On 2017/02/09 13:07:29, kdzwinel wrote: > It does help! Thank you. > > I adopted your code and fixed issues #1 and #2. Unfortunately, I run into two > more problems: > > 3) https://crbug.com/690255 - this is minor and will be handled separately > 4) previewed classes are not getting discarded when inspected node changes. See: > https://i.imgur.com/GUz6ZTF.gif . Whenever inspected node changes, > ClassesPaneWidget._update is called, however we no longer have access to the > previously inspected node at this point. Maybe you'll have an idea how to get > around this? Yeah, for this you need either: - store current node in ClassesPaneWidget - send the old value in UI.Context's flavorChangeListener(). This will give you a place to do cleanup; both approaches work for me, the first will be probably simpler. > > Cheers, > Konrad
It would be really nice to have this! Let me know if I can help you somehow
On 2017/02/18 at 04:48:27, lushnikov wrote: > It would be really nice to have this! Let me know if I can help you somehow Sorry for a delay. I've decided to go with updating FlavorChanged event to include both previous and new context. This required modifying addFlavorChangeListener callbacks all over the place, but seems to work well. However, I've found two more issues: - fast typing causes a race condition in ClassesPaneWidget._installNodeClasses . Same node is being added and removed to the _mutatingNodes array with each stroke and at some point it trips over causing current value from the input field to be added to the list of active classes ( see https://i.imgur.com/ToWfAhb.gif ). I was thinking about adding some kind of throttling on _onTextChanged class, WDYT? - before this CL, text in the 'Add new class' input wasn't cleared when context/node has changed, but with the preview capability IMO it makes sense to clear it. So I went ahead and implemented it. Unfortunately, clearing that input fails in one scenario. If I put some text into that input, hide the classes pane widget (by clicking on the '.cls' button) and reload the page being inspected, I get a "The given range isn't in document." error in TextPrompt.moveCaretToEndOfPrompt ( see https://i.imgur.com/HAeoFpC.gif ). I can add a check in that method that tests if `this._element` exists in the document (`document.body.contains(this._element)`), but maybe there is a more elegant way of solving this? Thank you for your help!
On 2017/02/21 23:47:19, kdzwinel wrote: > On 2017/02/18 at 04:48:27, lushnikov wrote: > > It would be really nice to have this! Let me know if I can help you somehow > > Sorry for a delay. I've decided to go with updating FlavorChanged event to > include both previous and new context. This required modifying > addFlavorChangeListener callbacks all over the place, but seems to work well. This adds a lot of changes around the code; can we have a locally stored node instead? Sorry for misguiding you in the first place. > However, I've found two more issues: > > - fast typing causes a race condition in ClassesPaneWidget._installNodeClasses . > Same node is being added and removed to the _mutatingNodes array with each > stroke and at some point it trips over causing current value from the input > field to be added to the list of active classes ( see > https://i.imgur.com/ToWfAhb.gif ). I was thinking about adding some kind of > throttling on _onTextChanged class, WDYT? Throttling will make race condition less visible, but won't eliminate the root cause. This seems like a re-enterability issue (issuing new calls while the previous haven't finished yet). Can we fix this instead? > > - before this CL, text in the 'Add new class' input wasn't cleared when > context/node has changed, but with the preview capability IMO it makes sense to > clear it. So I went ahead and implemented it. Unfortunately, clearing that input > fails in one scenario. If I put some text into that input, hide the classes pane > widget (by clicking on the '.cls' button) and reload the page being inspected, I > get a "The given range isn't in document." error in > TextPrompt.moveCaretToEndOfPrompt ( see https://i.imgur.com/HAeoFpC.gif ). I can > add a check in that method that tests if `this._element` exists in the document > (`document.body.contains(this._element)`), but maybe there is a more elegant way > of solving this? Looks like a bug in TextPrompt to me > > Thank you for your help!
On 2017/02/22 at 21:41:49, lushnikov wrote: > On 2017/02/21 23:47:19, kdzwinel wrote: > > On 2017/02/18 at 04:48:27, lushnikov wrote: > > > It would be really nice to have this! Let me know if I can help you somehow > > > > Sorry for a delay. I've decided to go with updating FlavorChanged event to > > include both previous and new context. This required modifying > > addFlavorChangeListener callbacks all over the place, but seems to work well. > This adds a lot of changes around the code; can we have a locally stored node instead? > Sorry for misguiding you in the first place. Agreed, I've added `_previousTarget`. > > > However, I've found two more issues: > > > > - fast typing causes a race condition in ClassesPaneWidget._installNodeClasses . > > Same node is being added and removed to the _mutatingNodes array with each > > stroke and at some point it trips over causing current value from the input > > field to be added to the list of active classes ( see > > https://i.imgur.com/ToWfAhb.gif ). I was thinking about adding some kind of > > throttling on _onTextChanged class, WDYT? > Throttling will make race condition less visible, but won't eliminate the root cause. > This seems like a re-enterability issue (issuing new calls while the previous > haven't finished yet). Can we fix this instead? I changed it, so that if setting attribute is in progress next setAttributeValue call will be postponed. > > > > > - before this CL, text in the 'Add new class' input wasn't cleared when > > context/node has changed, but with the preview capability IMO it makes sense to > > clear it. So I went ahead and implemented it. Unfortunately, clearing that input > > fails in one scenario. If I put some text into that input, hide the classes pane > > widget (by clicking on the '.cls' button) and reload the page being inspected, I > > get a "The given range isn't in document." error in > > TextPrompt.moveCaretToEndOfPrompt ( see https://i.imgur.com/HAeoFpC.gif ). I can > > add a check in that method that tests if `this._element` exists in the document > > (`document.body.contains(this._element)`), but maybe there is a more elegant way > > of solving this? > Looks like a bug in TextPrompt to me As far as I understand we are trying to move the caret to the end of the text when setText is called. This fails if TextPrompt is detached from the DOM. I was considering not calling the moveCaretToEndOfPrompt when the text is cleared (`setText('')`), but I'm not sure if I'm not missing something here. Also, as it turns out, "The given range isn't in document." isn't really an error, but rather a DevTools warning: https://bugs.chromium.org/p/chromium/issues/detail?id=697443 . Not sure what's the best option here, I'd appreciate a hint. > > > > > Thank you for your help!
https://codereview.chromium.org/2646283002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2646283002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:100: this._prompt.setText(''); to avoid that issue with selection you had, let's call this only in case of classesPane.isShowing(). Would it work for you? https://codereview.chromium.org/2646283002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:216: if (this._uncommitedValues.has(node)) { this complicates the _installNodeClasses method, making it behavior unpredictable. A better approach would be to use common.Throttler to throttle calls to the _installNodeClasses. If you change the _installNodeClasses to return promise on it's complition, then you'll be able to just use: this._throttler.schedule(this._installNodeClasses.bind(node, classes)) everywhere instead of: this._installNodeClasses(node, classes); This should eliminate the need for this._uncommittedValues
Thanks for help, I finally found some time to push that further. Besides fixing previously found bugs I had to update _onKey method since I managed to break existing solution in a new way (clicking ESC was clearing the input w/o updating the node). https://codereview.chromium.org/2646283002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2646283002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:100: this._prompt.setText(''); On 2017/03/07 at 02:55:12, lushnikov wrote: > to avoid that issue with selection you had, let's call this only in case of classesPane.isShowing(). Would it work for you? We will avoid the error this way, but introduce a slightly weird behaviour - see https://imgur.com/a/XGRj1 (field should have been cleared). Meanwhile, I came up with different fix and done what `_onKeyDown` is doing - circumvented the `this._prompt` and cleared `this._input` directly. https://codereview.chromium.org/2646283002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:216: if (this._uncommitedValues.has(node)) { On 2017/03/07 at 02:55:12, lushnikov wrote: > this complicates the _installNodeClasses method, making it behavior unpredictable. > > A better approach would be to use common.Throttler to throttle calls to the _installNodeClasses. > > If you change the _installNodeClasses to return promise on it's complition, then > you'll be able to just use: > > this._throttler.schedule(this._installNodeClasses.bind(node, classes)) > > everywhere instead of: > > this._installNodeClasses(node, classes); > > This should eliminate the need for this._uncommittedValues Thanks, I haven't seen common.Throttler before - useful!
https://codereview.chromium.org/2646283002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2646283002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:55: text = ''; i don't quite follow the logic here. The empty text variable might not be used since there's a fast return when there's no node. Are you sure you don't want to clear event.target.textContetn in this case? https://codereview.chromium.org/2646283002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:102: this._installNodeClasses(this._previousTarget); you probably want to flush throttler here to guarantee cleanup. Probably you'll need a third argument "immediately" to keep all throttler operations scoped to the _installNodeClasses functions. https://codereview.chromium.org/2646283002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:208: _setClassValue(node, value) { let's add jsdoc
https://codereview.chromium.org/2646283002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2646283002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:55: text = ''; On 2017/03/17 at 17:41:19, lushnikov wrote: > i don't quite follow the logic here. The empty text variable might not be used since there's a fast return when there's no node. Are you sure you don't want to clear event.target.textContetn in this case? Good catch! I've rearranged the code so that textContent is cleared before the fast return. https://codereview.chromium.org/2646283002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:102: this._installNodeClasses(this._previousTarget); On 2017/03/17 at 17:41:19, lushnikov wrote: > you probably want to flush throttler here to guarantee cleanup. Probably you'll need a third argument "immediately" to keep all throttler operations scoped to the _installNodeClasses functions. Updated, PTAL if I read your comment correctly.
Description was changed from ========== ClassesPaneWidget: Add ability to quickly preview autocompleted CSS classes. In action - https://i.imgur.com/yucn4TI.gif 📽 BUG=683623 ========== to ========== DevTools: ClassesPaneWidget - Add ability to quickly preview autocompleted CSS classes. In action - https://i.imgur.com/yucn4TI.gif 📽 BUG=683623 ==========
thanks! Looks like we're heading the right way! https://codereview.chromium.org/2646283002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js (right): https://codereview.chromium.org/2646283002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:83: this._update(); you don't need _update() here, do you? https://codereview.chromium.org/2646283002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:100: _onFlavorChange(event) { let's name this _onSelectedNodeChanged https://codereview.chromium.org/2646283002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:200: if (additionalClasses) { it looks like you never want to call "_installNodeClasses" without additional classes (consider e.g. the _onClick handler here). Let's have the additionalClasses computed here right away https://codereview.chromium.org/2646283002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/elements/ClassesPaneWidget.js:208: this._updateNodeThrottler.flush(); i looked closer into the flush() method and it turned out to be broken. I'm killing it now, sorry for misguidance: https://codereview.chromium.org/2770523002/ To support the scenario you're fighting here, I'd suggest a different approach, which I illustrated here: https://codereview.chromium.org/2763833004/. Long story short, it accumulates pending node classes requests and then sends it to backend in a batch.
Thanks for all the tips! I updated my code based on yours, rebased it with master, did some refactoring in _onKeyDown and tested all the edge cases. Seems to be working fine now. PTAL.
Nice work! lgtm
The CQ bit was checked by lushnikov@chromium.org
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": 1491608430734920,
"parent_rev": "6a2cc0923f353e6ab781ba7ef21b664106145c1a", "commit_rev":
"56d5720cf9a277a61d859d6829bf1a5bfb7d845c"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: ClassesPaneWidget - Add ability to quickly preview autocompleted CSS classes. In action - https://i.imgur.com/yucn4TI.gif 📽 BUG=683623 ========== to ========== DevTools: ClassesPaneWidget - Add ability to quickly preview autocompleted CSS classes. In action - https://i.imgur.com/yucn4TI.gif 📽 BUG=683623 Review-Url: https://codereview.chromium.org/2646283002 Cr-Commit-Position: refs/heads/master@{#463104} Committed: https://chromium.googlesource.com/chromium/src/+/56d5720cf9a277a61d859d6829bf... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/56d5720cf9a277a61d859d6829bf... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
