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.
Details
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
Proactively adding @ashoat as a reviewer here to take a look at approach at high level before review churn
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? |
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:
- Go with textarea and dummy div.
- Go with custom textarea(div) and use window.getSelection()
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:
- We go with current solution based on textarea.
- 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?
lib/shared/mention-utils.js | ||
---|---|---|
9 ↗ | (On Diff #18574) | At this point I can see that there might be two issues with it:
|
62 ↗ | (On Diff #18574) | Have you tested a case where textarea contains huge amount of text (more than full screen of it)? |
lib/shared/mention-utils.js | ||
---|---|---|
9 ↗ | (On Diff #18574) |
We might consider doing some benchmarks and maybe optimise this Regex, but my understanding is they are pretty efficient anyway.
|
62 ↗ | (On Diff #18574) | Yeah, sure, it work as expected. |