-
Notifications
You must be signed in to change notification settings - Fork 7.6k
[cmv3] Redo in css file may cause rendering issue in inline editor. #2782
Comments
@peterflynn -- would you have bandwidth to look at this? Could be a doc/editor syncing issue. |
Investigating... |
Reviewed. |
Still investigating. Looking more like a CM bug -- as far as I can tell, the syncing is doing the right thing, and the inline CM instance's getText() is correct... but the lines just don't get rendered. I don't see anything suspicious with the batching yet either. |
I also turned off the CSS code hint provider since it pops up during these steps -- but it still repros without the hints too. |
Notes on the bug's behavior:
|
Here are repro steps for Raymond's bug:
The edit does not appear in the master editor for main.css -- until you resize the window or click on the line that was changed. Also, when you click on the line it creates a selection unexpectedly, as if either the mouseup or mousedown position got miscalculated. |
If I back out #2778, Raymond's bug no longer repros. I believe the same problem would have occurred in v2 if we refreshed less often: even in v2, if updateDisplay() runs while hidden or orphaned, it no-ops and the list of changes accumulated by the operation is dropped on the floor (that is, the text model is updated but there's no longer any record of how to update the display to match). Only a refresh() can bring the display back into sync after that. Refreshing more often (by backing out #2778) does not fix the original bug with the inline editor, however. There might be a separate bug there with our own measurement code not getting re-kicked. |
But I'm assuming we have to ensure that any time an editor becomes visible, we call refresh(). Backing out NJ's change would guarantee this in most cases, but there may be a new v3-specific issue of inline editors that have scrolled out of view (in v2 this wasn't an issue since they weren't virtualized). I wonder if we should try to make our inlines non-virtualized in v3 too, at least for now? Have we tested the colorpicker to see if it too has problems updating while hidden? Also, ideally rather than backing out NJ's change entirely we'd just tweak it to ensure an editor is refresh()ed exactly once per show. (I think before NJ's change we sometimes did multiple refreshes for no good reason). |
We actually already have code that's supposed to handle this case. When CM re-adds a line widget to the DOM, it fires a I wonder if what might be happening, though, is that |
Actually, that doesn't necessarily make sense. Setting a fixed height on the outer widget doesn't actually constrain the child editor to that height--it should just clip it, I think (if the editor is |
It turns out that there was indeed an issue in the |
Here's another case, related to the master-editor bug @RaymondLim found yesterday. Like that bug, and unlike the original inline-editor case, this is also fixed simply by rolling back NJ's EditorManager change:
Result: empty inline editor is visible, but pressing Esc doesn't close it. If you resize the window, the inline editor disappears. |
Notes on the inline editor issue, for which we still don't know of a fix:
|
Discovered a couple more things.
So, the question now is where this bogus |
OK, I think I've tracked down where the bogus
It's not entirely clear why this doesn't cause a problem on the first switchback (after the initial undo), but it seems to be that in that case, the (bogus) So, it seems like there are really two bugs here: the caching of the bogus line heights, and the attempt to do scrolling while the view is invisible. I'm going to see if avoiding the scroll in that case fixes the problem. If so, it might be a reasonable patch. |
Filed codemirror/codemirror5#1236 on the scrolling issue. We should consider filing the bogus line height issue as well, but need to think about how to phrase that. |
FBNC to @RaymondLim |
Fix confirmed in cmv3 branch. Will regress in master and close at that time. |
…g the editor. Addresses some cases in #2782.
…oll-pos * origin/master: (236 commits) Update README.md Update README.md Update README.md Clarified comment Change string literals for resizeEditor() arg to constants Cleaned up refresh code and made it so we always refresh after showing the editor. Addresses some cases in #2782. Update CodeMirror SHA. For real this time. Update CodeMirror SHA Update CodeMirror SHA to brackets-sprint20 branch, which contains a temporary cherry-pick of a fix from upstream and avoids other fixes we don't want yet. Update CONTRIBUTING.md fix exception when opening empty folder and file open Remove extra divider from help menu. Reverting to old screenshot since the styling of the new screenshot doesn't match the (reverted) background. Updated de to match en Getting Started CSS dimgray background Turn off newline creation when autoclosing tags Set text cursor. Update comments. Only skip refresh if the width hasn't changed prettied it up a bit ... Conflicts: src/editor/EditorManager.js src/search/QuickOpen.js
Result: Only the second inline editor is in-sync with main.css and the top inline editor becomes blank. You still see the rule list and file name link though, but the css content is gone.
The text was updated successfully, but these errors were encountered: