codemirror / dev

Development repository for the CodeMirror editor project
https://codemirror.net/
Other
5.85k stars 371 forks source link

Using css transform results in incorrect behavior #324

Closed bryanlb closed 11 months ago

bryanlb commented 3 years ago

When attempting to use the editor inside of another element that has a css transform applied, the tooltips end up being incorrectly calculated on the x/y.

<div id="editor" style="transform: translate3d(0px, 0px, 0px);">
  <div class="cm-wrap cm-focused ͼ1 ͼ2 ">
    <div class="cm-scroller">

This can be replicated on the demo site here https://codemirror.net/6/ by doing an inline translate/rotate on the #editor div.

marijnh commented 3 years ago

This is a known problem with anything getClientRects-based—the reported coordinates don't reflect transforms. I'm not aware of any solution for this, so as it is transforming the editor is simply not something that's supported.

marijnh commented 3 years ago

It will also break drawSelection. And probably a lot of other things. I did a bit of research on this today, and this mozilla hacks post from 2014 acknowledges the problem, talks about a new standard that addresses it. In typical web fashion, that standard was never implemented in any browser.

So it doesn't look like it's going to be easy to address this in CodeMirror. There are apparently hacks that involve doing your own matrix transforms in code, but they don't appear to always work so I'm not terribly motivated to dig into that right now.

bryanlb commented 3 years ago

I was able to figure out an ugly hack that kinda-worked for my use case using the theme system. My use case has a wrapper element that has a fixed transform applied to it, so I was able to measure the element clientWidth and viewport innerWidth to calculate the amount I was off by. I then can use this in the theme system to set a margin on the $tooltip to correctly position it. This approach has a bunch of gross edge cases, and I didn't end up shipping this to production.

yxlolxy commented 3 years ago

I have the same problem! we use the codemirror in a dialog with a transform css property, and then the position makes a mistake ,beacause tooptip extension uses a fixed css layout but fixed layout will be affeced by transform property, check web mdn

fabianmichael commented 3 years ago

I just stumbled upon the DOMMatrix class, while doing research on a different topic. It is available in every modern browser (not IE 11) and seems to help against this problem, without having to implement your own parser for the translate property:

https://stackoverflow.com/questions/42267189/how-to-get-value-translatex-by-javascript/42267468

marijnh commented 3 years ago

Thanks, that an interesting bit of information. It seems possible to account for simple transforms with that relatively well, but as soon as you have anything where the transform's origin is relevant, that falls apart again, and you need a whole bunch of extra complexity to encode the origin into your matrix. Also nested transforms make things more tricky.

I'd be open to adding a hook that allows people to transform coordinates, if someone is interested in that, but I think doing this in the library is too much extra (probably fragile) code.

fabianmichael commented 3 years ago

I'd be open to adding a hook that allows people to transform coordinates, if someone is interested in that, but I think doing this in the library is too much extra (probably fragile) code.

I guess you’re right. This should also not be a typical use-case for most modern interfaces. It probably affects mostly dialogs, that have been vertically centered using the top: 50%; translateY(-50%) trick. Nowadays, there are much better ways of achieving such a behavior, using either Grid or Flexbox.

jdnarvaez commented 2 years ago

Is there an update on this? Would it be possible to expose a hook to allow for consumers to specify their own transform?

trusktr commented 2 years ago

You don't need a hook, you can apply CSS transform.

maxdavid commented 2 years ago

Hey! Running into problems with this for a project and it's kind of a major blocker for me.

In my case, I only need the tooltip functionality. Just changing line 109 of tooltip.ts in @codemirror/view to always set position to 'absolute' is enough to completely solve my problem.

@marijnh any chance you'd be willing to expose that positioning as an option we could pass somewhere to override the existing logic?

marijnh commented 2 years ago

@maxdavid The line you're linking already points at the solution to your problem.

maxdavid commented 2 years ago

My god, you've truly thought of everything. Thank you so much and congrats on the 6 stable release!

inca commented 2 years ago

As others pointed out, modern applications often use all sorts of transforms (scaling included), so "not supported" can actually be a very bad verdict.

6905bd57f6171e48ceb5b426c4dbbc8e

In my case I know exactly what the transform is, so I could apply a reverse transform to some of the elements — if only I knew where and what to apply.

inca commented 2 years ago

Fwiw with "codemirror": "^6.0.1" the partial (and very hacky) workaround might look as follows:

If you're lucky you'll get something like this, which still has problems, but at least allows you to see where the typing occurs.

feb242b42e045d1f115e8beb394fa445

I may end up just throwing away everything else which is incompatible with zooming (e.g. current line highlighting, tooltips, rectangular selection, etc, etc). All these fancy bells and whistles are secondary to zooming, which is pretty essential to our application.

mgmeyers commented 2 years ago

I'd be open to adding a hook that allows people to transform coordinates, if someone is interested in that, but I think doing this in the library is too much extra (probably fragile) code.

Just want to cast my vote for this. In my case, I'm using CM in a container with translate and scale applied. These are known values that I could use to transform coordinates if such a mechanism existed.

marijnh commented 2 years ago

I did a bit more research on this today, but it seems transformations, even simple translate/scale ones, are definitely not described by just their rotation and scale, but also by their origin. Accounting for that would push the (already involved) code around positioning to a level of complexity that I'm not currently comfortable with, and require more implementation and testing work than I'm able to allocate for this in the near future.

Notes:

nirtamir2 commented 1 year ago

I had the same issue when using codemirror with radix-ui popover which positioned fixed with transform. The autocompletion open in the wrong place

image

https://github.com/uiwjs/react-codemirror/issues/426

Here is a reproduction codesandbox.io/s/interesting-danny-xixf6p?file=/pages/index.tsx

mgmeyers commented 1 year ago

Just in case it's helpful for others: the workaround we ended up going with that fixed all of the issues related to scale/transform was to put the codemirror instance in an iframe.

nedgrady commented 1 year ago

Just in case it's helpful for others: the workaround we ended up going with that fixed all of the issues related to scale/transform was to put the codemirror instance in an iframe.

Got an example of this? We're having a similar issue to @nirtamir2 , using react-modal and the @uiw/react-codemirror component.

alex-shortt commented 1 year ago

Just in case it's helpful for others: the workaround we ended up going with that fixed all of the issues related to scale/transform was to put the codemirror instance in an iframe.

Worked like a charm, good find! If you're using react you can follow this component structure: https://stackoverflow.com/questions/34743264/how-to-set-iframe-content-of-a-react-component

devnaumov commented 1 year ago

Any other workarounds for this problem? Iframe is not an option. Seems like it worked perfectly fine in v5

marijnh commented 1 year ago

Seems like it worked perfectly fine in v5

I don't think that's true.

Wroud commented 1 year ago

you can add the extension tooltips to solve this issue:

tooltips: tooltips({
    position: 'absolute' | 'fixed',
    parent: document.body,
})
Wroud commented 1 year ago

The problem with fixed position, that its behaviour is different from absolute https://developer.mozilla.org/en-US/docs/Web/CSS/Containing_block

The element is removed from the normal document flow, and no space is created for the element in the page layout. It is positioned relative to the initial containing block established by the viewport, except when one of its ancestors has a transform, perspective, or filter property set to something other than none (see the CSS Transforms Spec), or the will-change property is set to transform, in which case that ancestor behaves as the containing block. (Note that there are browser inconsistencies with perspective and filter contributing to containing block formation.) Its final position is determined by the values of top, right, bottom, and left.

This value always creates a new stacking context. In printed documents, the element is placed in the same position on every page.

marijnh commented 1 year ago

Okay, I think I have a workable framing for this. As with attached patches, the view supports translation and scaling. Other types of transformation are probably never going to be supported, but translation is easy, and I believe I've added the necessarily logic to make scaling work properly.

This requires keeping a scale (which can be determined by dividing getBoundingClientRect().height by offsetHeight without any complicated matrix math or style querying) and running all accesses to things like clientWidth/offsetHeight/scrollTop (!) thought that scale when reading them, and transforming values back through that scale whenever any pixel-measurement styles are set. This is, as you can guess, pretty tedious and error-prone, but I'll try to make sure to maintain that convention in any further changes to the library.

Let me know if you've tested this and found some issue.

xr0master commented 1 year ago

Sounds too complicated, maybe it's better to use simple parent.getBoundingClientRect().top + element.offsetTop.

@marijnh Ah, I see that the @codemirror/view version 6.18 fixes this issue!

andsav commented 1 year ago

@marijnh thanks for addressing this issue! Any chance the fix could be backported to codemirror5? Even any pointers of what should be patched to get this working in CM5 would be appreciated.

marijnh commented 1 year ago

Any chance the fix could be backported to codemirror5?

Not really. That's a completely different codebase, so 'backporting' isn't something that's straightforward, and maintenance for that project is down to critical bug fixes and reviewing simple PRs.

marijnh commented 11 months ago

Closing this. Support for more complicated transforms is not planned.