-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
@marcelgerber++ 👍 |
Tagging @swmitra |
@marcelgerber I know these tests failed before your update. But do you think we can fix them in this PR to make sure we are not introducing regressions? It would awesome to have peace of mind. |
It would be nice to explicitly call out which version of CodeMirror we are updating to. :) |
@marcelgerber I found the particular change in CodeMirror that is causing this regression. I am going to dig in a little bit more to see if I can find the reason why the change was made. The question is whether or not that is the correct behavior in CodeMirror. |
Well, this thread has the history of why the change of was made. codemirror/codemirror5#3658 |
@marcelgerber I found what we have to change on our side. The changes in codemirror actually seems to make things more consistent in behavior. I am still running tests. https://github.com/adobe/brackets/blob/master/src/editor/Editor.js#L1507 |
@marcelgerber - BTW - if you want to see a visual of the behavior: 1 go to https://codemirror.net/index.html
3 Select anything spanning multiple lines.
4 Now clear both markers to see the selected text in the editor.
5 Checkout the selected range.
That selects the whole line regardless of what's around, which is the behavior we want when we are hiding lines as the unit test does. |
I found an odd behavior in CodeMirror that is causing our particular unit test failures. I have provided information over on the CodeMirror side. |
@marcelgerber I think the issue is fixed in the latest code mirror. https://github.com/codemirror/CodeMirror/releases/tag/5.13.0. Gotta bring it down and test it. |
Now is exit a new version of Code Mirror https://github.com/codemirror/CodeMirror/releases/tag/5.13.2 |
Thanks for pointing that out! |
@marcelgerber CodeMirror had a couple of changes around ranges that fixed the issue :) |
I pulled your branch. I am running today with it and if things go well, we can merge? |
codemirror/codemirror5@c65244d changed the behavior in this case; the newly inserted braces are no longer part of the resulting selection.
208a19f
to
8675647
Compare
Surely we can! |
things seem to work ok. I will merge now so that more people can get to test this update early. Thank you @marcelgerber !! |
For #12031.
Several tests fail, but most of them do fail on master, too, so I suppose it's not my fault.
I, though, had to disable 3 EditorCommandHandlers tests which have to do with how CM handles atomic markers mid-text. There was a slight behavior change in codemirror/codemirror5@1fae537 which makes these tests fail.
This change could theoretically affect inline editors, but in my manual testing I didn't face any issues.
For more reference on this change, see codemirror/codemirror5#3699.
Fixes #11905, #11683, and a whole lot more.