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

[issue 460] Support variable-width text #1608

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bvibber
Copy link

@bvibber bvibber commented Sep 15, 2013

Updated from 2011 patch by fjakobs:
36ebe6c

  • removes old double-width CJK handling
  • replaces monospace assumptions with measurements of actual text
  • appears to work for Latin, CJK, Thai, Malayalam in ad-hoc testing
  • does NOT fully handle RTL text issues (Hebrew, Arabic)

Updated from 2011 patch by fjakobs:
ajaxorg@36ebe6c

* removes old double-width CJK handling
* replaces monospace assumptions with measurements of actual text
* appears to work for Latin, CJK, Thai, Malayalam in ad-hoc testing
* does NOT fully handle RTL text issues (Hebrew, Arabic)
var space = new Array(c.length+1).join(self.SPACE_CHAR);
return "<span class='ace_invisible'>" + space + "</span>";
} else {
return "&#160;";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use self.SPACE_CHAR here and above? (Please note that I came here from MediaWiki bug 54136 and I have no idea how this code works, just reading the changes :) )

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading things right, SPACE_CHAR is the replacement character to be used in "show invisibles" mode -- it's a mid-dot character. Non-breaking space ( ) sounds right for non-showing mode.

@MatmaRex
Copy link

Testing on the kitchen sink page on Opera 12, this seems to break cursor positioning in funny ways when I enable soft wrap.

@bvibber
Copy link
Author

bvibber commented Sep 16, 2013

Yeah, multiline / soft wrap doesn't work (per notes on the original patch), we may just have to live with that for Wikipedia for now but that'd be nice to fix proper... there's also a problem with the tab character that definitely needs fixing.

@nightwing
Copy link
Member

Thank you very much for updating this patch!
It isn't mergeable yet because

  1. tests need updating after double-width CJK handling removal
  2. it needs to properly reset cache when settings like fontSize/theme/showlnvisibles change
  3. would be good to not do slow dom measurements in common case of monospace english text.

also i think cursor movement isn't quite right for languages like Tamil, but it can be fixed later.

@bvibber
Copy link
Author

bvibber commented Sep 17, 2013

Awesome!

  1. can probably be done by doing a quick regex check; if you've got all-ASCII or such then switch to the fast-path monospace behavior.

@fjakobs
Copy link
Contributor

fjakobs commented Sep 18, 2013

@Brion Thanks for reviving my varchar patch.

The tricky part about the monospace fast path is to detect if the font is really monospace. Also with the varchar patch in place we can finally allow bold styles and different fonts in the themes. If that is used it would be very hard to implement a proper fast path.

As for softwrap + varchar: I think it can be done but it might become fairly expensive. Finding an efficient solution will be tricky. We can't unfortunately move that computation into a worker because measuring sizes requires DOM access. We could start by assuming monospace and only if the text comes into view reflow the visible areas with the real text sizes.

@kasperpeulen
Copy link

This pull request completely solves my problems with inserting unicode math symbols that are not supported by monospace fonts. I would really like to use this patch at websites like stackedit.io or sharelatex.com. I asked sharelatex.com if it possible to use this patch, but they are waiting until this patch is merged.

About problem 3), what about adding an option editor.enablevariableWidthText(true); ?

@@ -1348,15 +1350,47 @@ var VirtualRenderer = function(container, theme) {
return {row: row, column: col, side: offset - col > 0 ? 1 : -1};
};

this.$findColumn = function(row, width) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me like this functionality would make more sense inside the TextLayer itself. The rendered would instead ask the TextLayer what column and row a particular x,y belong to. You could add an alternative iteration/exit condition of measureText that would measure up to a given width and return the column.

@digitalmoksha-machine
Copy link

Is there any hope of getting this patch merged? I need to support various unicode character (diacritics, telugu, hindi, etc) and this is constantly biting me. It really becomes impossible to use the editor in these circumstances. This patch seems to fix the problem. Really appreciate the work @Brion and @fjakobs have done on this.

+1 to get this finished up and merged

@camertron
Copy link
Contributor

+1 to getting this merged. Pretty please?

@nightwing
Copy link
Member

This doesn't work with showInvisibles, tab characters, and wrapped lines, first two issues are easy to fix, but wrapping requires more work. Would this be useful for you without wrapping support?

@camertron
Copy link
Contributor

@nightwing Yes, but wrapping support is also a must-have, at least for us. Thanks for your quick response :)

@camertron
Copy link
Contributor

If you can point me in the right direction, I might be able to lend a hand fixing wrapping.

@nightwing
Copy link
Member

@camertron any help would be welcome of course.
I do not know yet how to fix wrapping, probably renderWrappedLine in textlayer.js needs to be changed to draw whole line in one div and allow browser to handle wrapping. Since this way we won't have exact height for scrollbar, virtual_renderer will have to measure visible lines and update scrollbar when new lines are drawn, which is a bit tricky since it will affect scrolling animation, scrollintoview etc.

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

Successfully merging this pull request may close these issues.

9 participants