Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACE line heights since 1.13.1 #5012

Closed
jmthomas opened this issue Dec 12, 2022 · 11 comments · Fixed by #5052
Closed

ACE line heights since 1.13.1 #5012

jmthomas opened this issue Dec 12, 2022 · 11 comments · Fixed by #5052

Comments

@jmthomas
Copy link

Describe the bug

I assume the change in #4996 results in a different calculation of line heights. Since 1.13.1 (I confirmed 1.13.0 does not have this issue) the line heights are incorrect and result in the text being smashed together. Manually requesting a browser zoom in or out will re-render the lines correctly.

Expected Behavior

Previously in 1.13.0 this rendered correctly.

Current Behavior

Here's an example of the initial display:

Screenshot 2022-12-12 at 11 37 15 AM

Reproduction Steps

I'm mounting this within a Vue.js component but it's pretty straightforward except perhaps it's running on the mounted life cycle method. Not sure if that's resulting in a browser environment that can't figure out the proper line height?

<template> 
 <pre id="editor"></pre>
</template>

<script>
import * as ace from 'ace-builds'
import 'ace-builds/src-min-noconflict/mode-text'
import 'ace-builds/src-min-noconflict/theme-twilight'
import 'ace-builds/src-min-noconflict/ext-language_tools'
import 'ace-builds/src-min-noconflict/ext-searchbox'
export default {
  mounted: function () {
    this.editor = ace.edit('editor')
    this.editor.setTheme('ace/theme/twilight')
    this.editor.setValue(this.definition)
    this.editor.clearSelection()
    this.editor.focus()
  },
}
</script>
<style scoped>
#editor {
  height: 50vh;
  width: 75vw;
  position: relative;
  font-size: 16px;
}
</style>

Possible Solution

No response

Additional Information/Context

Vue mounted lifecycle hook says:

A component is considered mounted after:

All of its synchronous child components have been mounted (does not include async components or components inside trees).

Its own DOM tree has been created and inserted into the parent container. Note it only guarantees that the component's DOM tree is in-document if the application's root container is also in-document.

This hook is typically used for performing side effects that need access to the component's rendered DOM

Ace Version / Browser / OS / Keyboard layout

1.13.1 / Chrome (107.0.5304.87) / Mac OS 13 / Querty

@andrewnester
Copy link
Contributor

@andredcoliveira could you please take a look?

@andredcoliveira
Copy link
Contributor

andredcoliveira commented Dec 14, 2022

v1.13.1 did change the way line heights (and widths) are calculated via commit e57a9d9—which happens here.

Before:

    this.$measureSizes = function(node) {
        var size = {
            height: (node || this.$measureNode).clientHeight,
            width: (node || this.$measureNode).clientWidth / CHAR_COUNT
        };

        // Size and width can be null if the editor is not visible or
        // detached from the document
        if (size.width === 0 || size.height === 0)
            return null;
        return size;
    };

After:

    this.$measureSizes = function(node) {
        node = node || this.$measureNode;

        // Avoid `Element.clientWidth` since it is rounded to an integer (see
        // https://developer.mozilla.org/en-US/docs/Web/API/Element/clientWidth).
        // Using it here can result in a noticeable cursor offset for long lines.
        const rect = node.getBoundingClientRect();
        const charSize = {
            height: rect.height,
            width: rect.width / this.charCount
        };

        // Size and width can be null if the editor is not visible or
        // detached from the document
        if (charSize.width === 0 || charSize.height === 0)
            return null;
        return charSize;
    };

This change was made because Element.clientHeight and Element.clientWidth are rounded to the nearest integers, while Element.getBoundingClientRect provides more precise—fractional—values.

So, I do see a change in line heights by using the Vue snippet you provided and switching between ace-builds v1.13.0 (which results in integer line heights) and v1.13.1 (which results in fractional line heights). But I can't reproduce the "text being smashed together". It's surprising to me that v1.13.1 makes it worse for your use case since I would expect this to be a bigger issue with the rounding:

  • in v1.13.0, a line height that should be, e.g., 21.3333px would end up as 21px, resulting in 0.3333px being cropped
  • in v1.13.1, a line height that should be 21.3333px ends up as 21.3333px

As a side note, I changed the following line in your snippet:

this.editor.setValue(this.definition)  // this.definition is `undefined`

to:

this.editor.setValue('THESE\nARE\nSOME\nRANDOM\nWORDS')

Even without fully understanding the root cause, I can think of 2 potential mitigations:

  • Using Math.ceil when measuring character height
  • Reverting the height (but not the width) to using Element.clientHeight
    • Preferred since we know it used to be fine with this

@andredcoliveira
Copy link
Contributor

@jmthomas To clarify, have you confirmed that your reproduction snippet does reproduce the issue? Double checking since the screenshot you provided is not what the snippet results in.

@github-actions
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@Tiuipuv
Copy link

Tiuipuv commented Dec 23, 2022

I am also having this issue when upgrading from v1.13.0 to v1.13.1. Note that for me, it does not happen every time ace is attached to a div, it is only about 30% of the time (some sort of race condition). Note the very odd highlight too, when selecting the top two lines, both the height and width are off:
1 13 1

And below is what it looks like in 1.13.0 (correct)
1 13 0

My ace editor is instantiated in a SweetAlert2 dialog box, which animates in (width and height/scale of this parent might play a factor in the 'race' condition). Please let me know if you want me to try to produce a minimum viable example.

@jmthomas
Copy link
Author

jmthomas commented Dec 23, 2022

@andredcoliveira I can consistently reproduce this in our application where we pop open an edit dialog. What's interesting is that our script editor application doesn't experience this issue. If I change the page zoom level it regenerates the editor with the correct heights so this is something that's happening on initial render. I'll play with it some more but I don't know how to override internal Ace methods in my application so I'm simply observing behavior.

@jmthomas
Copy link
Author

jmthomas commented Dec 23, 2022

I can confirm adding a small delay await new Promise((r) => setTimeout(r, 300)) in our Vue mounted method solves the problem. Not sure if there's anyway to know what's rendered on your end but if anyone else runs into this issue try adding a delay or waiting until your viewport is rendered before creating the Ace editor.

@jmthomas
Copy link
Author

@Tiuipuv The odd highlight is because Ace miscalculated the width as well as the height. I saw something similar where I couldn't move the cursor past the end of the line and editing was really weird.

@Tiuipuv
Copy link

Tiuipuv commented Dec 23, 2022

@jmthomas Yeah I was figuring that may be the case. the reason I included it was because I was worried about @andredcoliveira's potential solution of:

This made me wonder if it would then highlight with the same oddity horizontally, but be fixed vertically. But who knows, I know very little about ace's code base. for now :)

@kemayo
Copy link
Contributor

kemayo commented Feb 3, 2023

We're seeing this one on Wikipedia as well, after we updated from an older Ace in mid-January. When editing articles we embed Ace as a "code" editor and for math formulas, both of which involve popping up a dialog containing the editor. We think it's that Ace's new size calculations fail if they run while that dialog opening animation is running, whereas the older method worked fine.

@nightwing
Copy link
Member

@kemayo thanks, the example you provided have helped to track down the issue. It is happening because getBoundingClientRect returns height in transformed coordinates and clientHeight in local coordinates. This same change have broken https://raw.githack.com/ajaxorg/ace-builds/master/demo/transform.html as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants