[CLOSED] [cmv3] Redo in css file may cause rendering issue in inline editor.

core-ai-bot commented 3 years ago

Issue by RaymondLim Saturday Feb 02, 2013 at 07:02 GMT Originally opened as https://github.com/adobe/brackets/issues/2782

Open index.html from Getting Started project.
Press Ctrl+E on h1 tag.
Press Ctrl+E on h2 tag that is immediately below the h1 tag.
Set cursor in the first inline editor (the one opened in step 2) and type some text.
Select main.css in the working set to bring it to the front.
Undo and then switch back to index.html. You will see both inline editors in sync with changes in main.css.
Switch back to main.css and redo.
Switch back to index.html.

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.

core-ai-bot commented 3 years ago

Comment by njx Wednesday Feb 06, 2013 at 05:09 GMT

@peterflynn -- would you have bandwidth to look at this? Could be a doc/editor syncing issue.

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Feb 07, 2013 at 22:51 GMT

Investigating...

core-ai-bot commented 3 years ago

Comment by gruehle Thursday Feb 07, 2013 at 23:36 GMT

Reviewed.

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Feb 07, 2013 at 23:56 GMT

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.

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Feb 08, 2013 at 00:23 GMT

I also turned off the CSS code hint provider since it pops up during these steps -- but it still repros without the hints too.

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Feb 08, 2013 at 00:59 GMT

Notes on the bug's behavior:

Repros with only the first inline editor open -- you can skip the part where you open a 2nd inline editor that's never touched.
If you resize the window a little bit, the inline editor's text reappears, and it's totally correct. The inline editor gets a little taller in the process.
If you click far to the right of where the code should be seen, or just beneath it (in the padding at the bottom of the inline widget), the same thing happens -- the widget grows taller & the text reappears. If you click on top of where the text should be, you get a blinking cursor in the middle of nowhere but that's it.
Does not repro if you don't switch back to main.html in step 6 (i.e. if you undo, then redo without switching editors in between). (Although on Raymond's machine this is apparently not true).
Still reproduces if you roll back the refresh() changes in #2778
Raymond is seeing another, possibly related bug: sometimes, when typing in the inline editor, changes are not reflected in the master editor until you click on/near the line where the change should have appeared. Then the change is rendered correctly. (Common thread: both seem to involve rendering problems with editors that were hidden when they received a change).

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Feb 08, 2013 at 01:15 GMT

Here are repro steps for Raymond's bug:

Open both index.html & main.css, and both to the working set. Make sure you actually visit both files so the editors get created right away.
Switch to index.html
Open inline editor & make edit
Switch back to main.css

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.

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Feb 08, 2013 at 02:05 GMT

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.

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Feb 08, 2013 at 02:08 GMT

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).

core-ai-bot commented 3 years ago

Comment by njx Friday Feb 08, 2013 at 05:21 GMT

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 redraw event, which should be handled by the handler in InlineTextEditor.onAdded() that triggers a refresh of the child editor. In theory, that refresh should then eventually cause an update event to be triggered on that same editor, which then is handled by the update.InlineTextEditor handler in InlineTextEditor.createInlineEditorFromText(), and that handler should then call sizeInlineWidgetToContents(). (Phew.)

I wonder if what might be happening, though, is that sizeInlineWidgetToContents() gets called too early in some case where we're switching back and forth between files, and applies a bad fixed height to the widget before its child editor has been refreshed (so then the child editor is now constrained to that height). If that's what's happening, maybe on a redraw event, we should clear the fixed height on the widget before refreshing the child editor.

core-ai-bot commented 3 years ago

Comment by njx Friday Feb 08, 2013 at 05:23 GMT

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 height: auto). So the child editor should be able to figure out its natural height anyway. But maybe there's something else going wrong with the redraw/refresh/update chain I outlined above.

core-ai-bot commented 3 years ago

Comment by njx Friday Feb 08, 2013 at 05:52 GMT

It turns out that there was indeed an issue in the redraw timing that caused #2806. However, my workaround for that issue doesn't seem to fix this issue (though it would probably fix the hypothetical case that@peterflynn was describing). There must be some other reason why the inline widget isn't refreshing properly when the editor is reshown.

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Feb 08, 2013 at 22:19 GMT

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:

Open inline editor on <h1> in Getting Started
Use 'Shift Line Down' to move the middle line down -- inline editor snaps shut
Reopen inline editor (now the rule has nothing between the {}s)
Go to the full CSS file
Undo
Go back to the full HTML file

Result: empty inline editor is visible, but pressing Esc doesn't close it. If you resize the window, the inline editor disappears.

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Feb 08, 2013 at 22:31 GMT

Notes on the inline editor issue, for which we still don't know of a fix:

CodeMirror has a display.cachedTextHeight field that caches the height of 1 display line. This value appears to only get used (and set) during document mutation handling, not during rendering -- see below.
When you peek at the inline editor in step 6, it is refreshed, which causes that cache to get nulled out
When the inline editor receives a change while hidden, the text height is re-requested (in updateLine(), called from updateDoc()). Since the editor is hidden it's unable to measure anything, and textHeight() returns 1. It leaves the cache nulled, but the returned 1px value is stuffed onto the line objects' line.height field -- which persists, almost like another layer of caching.
When the inline editor becomes visible later, we refresh it again. Refreshing re-renders all the HTML content, but rendering never looks at textHeight() / cachedTextHeight. It happily uses the incorrect 1px values stored on the line objects.
When the window is resized (the "workaround") something happens to reset the line objects' heights to correct values. However, afaict textHeight() is not being called and the cache remains nulled, so there are apparently two codepaths by which line.height's value is generated. (It's proving hard to track down where it's updated since updateLineHeight() is highly isolated from any notion of CM instances -- you get tons of calls into it for other editors and it's hard to tell which is which).
I also don't understand why this explanation doesn't hold for the full-size editor rendering issues. It may come down to the timing of refreshes, but who knows...

core-ai-bot commented 3 years ago

Comment by njx Monday Feb 11, 2013 at 23:34 GMT

Discovered a couple more things.

The main reason why the bogus text heights are messing things up is indeed that the visibleLines() calculation is incorrect. In general, though, this should fix itself at the point where lines get rerendered. However, in this case, visibleLines() returns a range that starts at the bottom of the visible text, instead of at the top...
The reason for that is because an apparently bogus scrollTop gets passed in as the top of the viewPort into updateDisplay() from endOperation(); it's 45 when it seems like it ought to be 0. This only happens in the refresh() case because refresh() explicitly tries to restore the last known scroll position, whereas resize() doesn't do this, so the scroll position ends up getting recaldulated in endOperation() by a call to calculateScrollPos().

So, the question now is where this bogus scrollTop is coming from--at some point it gets cached on the doc, which is where refresh() picks it up from.

core-ai-bot commented 3 years ago

Comment by njx Tuesday Feb 12, 2013 at 01:20 GMT

OK, I think I've tracked down where the bogus scrollTop is coming from. It actually happens much earlier--immediately during the first undo, while the inline editor is hidden for the first time. The sequence in the hidden inline editor is:

endOperation() is called
... which calls updateDisplay(), but that bails because it's invisible
... and also wants to scroll the cursor onto the screen, so it calls cursorCoords() to figure out where it is, then calls calculateScrollPos()
... but because the inline editor is hidden, clientHeight is 0, so calculateScrollPos() thinks the cursor is off the bottom of the screen
... so endOperation() sets the scrollTop too large. This is cached in the doc.
Later, when the inline editor becomes visible and we refresh it, refresh() scrolls the inline editor to the bogus cached scroll position.

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) visibleLines() calculation happens to return a range that actually includes part of the visible text, and then the visible range gets expanded to the whole doc, so it's okay. But the second time, the visibleLines() calculation returns an empty range at the end of the doc, and it only gets expanded back to immediately after the last visible line in the doc, probably due to the if (sawCollapsedSpans) logic.

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.

core-ai-bot commented 3 years ago

Comment by njx Tuesday Feb 12, 2013 at 01:46 GMT

Filed https://github.com/marijnh/CodeMirror/issues/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.

core-ai-bot commented 3 years ago

Comment by njx Tuesday Feb 12, 2013 at 01:48 GMT

FBNC to@RaymondLim

core-ai-bot commented 3 years ago

Comment by RaymondLim Tuesday Feb 12, 2013 at 02:00 GMT

Fix confirmed in cmv3 branch. Will regress in master and close at that time.

brackets-archive / bracketsIssues

[CLOSED] [cmv3] Redo in css file may cause rendering issue in inline editor. #2637