Page MenuHomePhabricator

[lib] Added getTextsOffsets function. Refactored mention-utils.js.
ClosedPublic

Authored by przemek on Nov 18 2022, 7:43 AM.
Tags
None
Referenced Files
F3382849: D5674.id18948.diff
Thu, Nov 28, 12:21 PM
F3382348: D5674.diff
Thu, Nov 28, 10:04 AM
F3381152: D5674.id18838.diff
Thu, Nov 28, 4:11 AM
Unknown Object (File)
Mon, Nov 25, 3:31 AM
Unknown Object (File)
Sun, Nov 24, 8:22 PM
Unknown Object (File)
Fri, Nov 22, 8:00 AM
Unknown Object (File)
Fri, Nov 22, 8:00 AM
Unknown Object (File)
Fri, Nov 22, 8:00 AM

Details

Summary

I've done changes to mention/@-ing Regex.
Also added getTextsOffsets function. It is kinda hacky, but I couldn't find better way to do it.
It is used to measure cursor offset in pixels from textarea top-left corner, so we can position
@ing typeahead overlay correctly.
Currently it doesn't take into account that overlay may not fit into window, so it can overflow into the left side of window, but it will be fixed in next diffs.
I wanted to put it up, as I'm not at work on Mon/Tue, but may already get some review.

Test Plan

Tested in next diffs.
Works correctly and displays overlay above currently typed "mention".

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

przemek edited the summary of this revision. (Show Details)
atul retitled this revision from [lib] Added getTextsOffsets function. Refactored menntion-utils.js. to [lib] Added getTextsOffsets function. Refactored mention-utils.js..Nov 18 2022, 1:43 PM

Proactively adding @ashoat as a reviewer here to take a look at approach at high level before review churn

ashoat requested changes to this revision.Nov 21 2022, 6:37 PM

Need more justification for this hacky approach

lib/shared/mention-utils.js
34–43 ↗(On Diff #18574)

Can you link to where you got this solution from? And can you link to a recent source that asserts this is the modern way to do this?

45–47 ↗(On Diff #18574)

Do we need to do this for literally every style?

This revision now requires changes to proceed.Nov 21 2022, 6:37 PM

Initially I got idea from this site: https://jh3y.medium.com/how-to-where-s-the-caret-getting-the-xy-position-of-the-caret-a24ba372990a.
I browsed through available resources and what people suggested was either that, or even more hacky solution using canvas. Some of solutions only gave offset of caret in characters count, but that's insufficient for us, as we need x,y position of caret in textarea.
Later on I found npm package that does it in a similar way:
https://github.com/component/textarea-caret-position

I figured it is better to do it ourselves than to include 3rd party package, expecially when I had code already written and working.

Regarding second question:
Probably not every style, but that way it is more readable, than copying 20 or 30 specific styles by hand. Also that way, any change to textarea in the future won't break this function.
I figured that it shouldn't be performance issue on web, just checked it and entire function call takes about 1ms, but it is run locally in user's browser, so that's fine I guess.

Both of those sources (Medium and NPM package) are very old. In my last comment, I requested that you "link to a recent source that asserts this is the modern way to do this". Requesting this again: can you find a more recent source (eg. last two years) that strongly asserts that this is the only way to do it?

(Keep in mind that each time we need an additional review cycle, it slows us down by a day. The more you can do to read your reviewers' comments carefully, the faster you will unblock yourself.)

Probably not every style, but that way it is more readable, than copying 20 or 30 specific styles by hand.

I don't agree... we should not be copying every single style over if we don't need to. This is similar to the feedback I gave here... you should never be doing something in code if you don't understand the potential implications of it.

than copying 20 or 30 specific styles by hand.

Do we really need 20/30 styles? I thought we just need the positions?

I figured it is better to do it ourselves than to include 3rd party package, expecially when I had code already written and working.

I generally agree it's better to do small things like this ourselves, and we definitely should not use the NPM package you linked. Its last release was in 2014.

Yeah, sorry for that. I thought I browsed through most of it and couldn't find anything interesting, that would differ significantly from what I've done.
This solution though seems good and much clearer than creating dummy div:
https://javascript.plainenglish.io/how-to-find-the-caret-inside-a-contenteditable-element-955a5ad9bf81
https://codesandbox.io/s/caret-coordinates-index-contenteditable-9tq3o?from-embed=&file=/src/index.js:577-591

Long story short, it utlises getClientRects() on Range object retrieved from Selection object. It allows us to get caret position in much simpler way. More on Ranges and Selections: https://javascript.info/selection-range

The more you can do to read your reviewers' comments carefully, the faster you will unblock yourself.

That's what I'm aiming for.

I think we should go with solution linked above, but answering your question. I found it neccessary to copy all styles, as the dummy div has to be rendered in a precisely same way as a textarea.
If we don't copy, say, padding and somebody sets padding on textarea, the overlay would not show in expected place.
The function should only be called when we find users matching to mentionRegex, so I found it wouldn't affect general performance significantly, but I might wrong.
Is performance an issue here or is there something else I don't understand?

After further reading on above solution I discovered one gimmick. We would either need to implement custom textarea i.e. we would need div(which we probably don't want) as window.getSelection() doesn't trigger correctly in textareas, because
they provide custom API (https://html.spec.whatwg.org/#textFieldSelection).

So far, I was not able to find a way to workaround it unfortunately.

After some research I was not able to find any site explicitly telling that it can't be done in another way, but the only two feasible solutions I see is:

  1. Go with textarea and dummy div.
  2. Go with custom textarea(div) and use window.getSelection()

Does this solution work? I think we need to do more searching here

No, unfortunately it does not, because of this:

After further reading on above solution I discovered one gimmick. We would either need to implement custom textarea i.e. we would need div(which we probably don't want) as window.getSelection() doesn't trigger correctly in textareas, because
they provide custom API (https://html.spec.whatwg.org/#textFieldSelection).

So far, I was not able to find a way to workaround it unfortunately.

Ah okay. I think we need to do more searching to figure out for sure that this is the best way forward. We should look to find a recent source that goes through the options, and we should consider each option. For instance, what if we replaced the textarea with a contenteditable div?

(Please prioritize finding a source online before investigating individual options)

I've made a quick search and it seems like most of the websites use contenteditable for forms that show e.g. typeahead. This approach has huge benefit of allowing formatting the text inside easily (e.g. bolding the mentions). Phabricator is somehow different, as it uses textarea and figures all the data using JX.TextAreaUtils (not sure what it is) and some complicated logic.

Hi @ashoat, I spent some time looking and tried my best to find some quality source , but couldn't find any. Most are old, low quality posts.
I think your's and Tomek's experience and opinions would be more valuable than those.
I see two possible solutions here:

  1. We go with current solution based on textarea.
  2. We spend some time refactoring ChatInputBar to use div with contentEditable enabled. That would require extra work as we would need to handle some more things manually.

I'd prefer to go with solution 1), so we can deliver feature before end of November ( as I'm almost ready with other diffs) and creating issues for that refactor (textarea -> content editable div).
When refactoring we can change getTextOffsets() as well to be less sketchy. It should be easy to do as it is relatively independent from other code.
Let me know wha you think?

This revision now requires review to proceed.Nov 24 2022, 9:40 AM
tomek added inline comments.
lib/shared/mention-utils.js
9 ↗(On Diff #18574)

At this point I can see that there might be two issues with it:

  1. (?:^(?:.|\n)*\\s+)|^ - It seems like if the text contains \n, the next 'sign' would be ^ so probably using \n here is not needed: (?:^.*\\s+)|^ probably will work the same
  2. Why this regex is ended with $? Will it work with mention being in the middle of the text? (It may, if we're checking only a substring against it)
62 ↗(On Diff #18574)

Have you tested a case where textarea contains huge amount of text (more than full screen of it)?

This revision is now accepted and ready to land.Nov 25 2022, 2:07 AM
przemek added inline comments.
lib/shared/mention-utils.js
9 ↗(On Diff #18574)

@tomek

  1. That part is needed so we only detect the last @ with some name prefix. In mine regex we go through all characters or newlines ( dot doesn't match newline thus(?:.|\n) is needed). We match it any number of times until we find last @ character. After that we match username prefix, that we use for search.

We might consider doing some benchmarks and maybe optimise this Regex, but my understanding is they are pretty efficient anyway.

  1. Your intuition is correct. We're running regex only on part of text (from start to cursor position). It works in a middle of text.
62 ↗(On Diff #18574)

Yeah, sure, it work as expected.

przemek marked an inline comment as done.

Renaming function