rugk / unicodify

✍️ A browser add-on (Firefox, Chrome, Thunderbird) that allows you to autocorrect common text sequences and convert text characters to a look like a special font.
https://addons.mozilla.org/firefox/addon/unicodify-text-transformer/?utm_source=github.com&utm_medium=git&utm_content=repo-about-description&utm_campaign=github
Other
11 stars 2 forks source link

Document/ask for problematic site-issues #62

Closed rugk closed 2 years ago

rugk commented 3 years ago

We could always ask on Mozilla's Discourse or Stack Overflow, but I think our chances of getting an acceptable solution there would be very low.

https://github.com/rugk/unicodify/issues/4#issuecomment-898336176

IMHO this is a good idea, @tdulcet, is not it? I mean, why do you think the chances are low? I guess Stackoverflow would be better though one can and maybe should also cross-post it/just copy it. IMHO we should try that, at least…

Affected issues

I guess we should tackle these site-issues for a v1.0 release:

https://github.com/rugk/unicodify/issues?q=is%3Aopen+label%3A%22help+wanted%22+label%3Asite-issue

Plan/TODO

tdulcet commented 3 years ago

IMHO this is a good idea, @tdulcet, is not it? I mean, why do you think the chances are low?

Umm, it is worth a try I guess… In my experience, the complex questions on these types of websites tend not to get good or sometimes any answers.

write Stackoverflow or so questions, as I e.g. did here, there are a lot of tags for WebExtensions

For #4, I believe our main question would be something like “How to get the caret position in ContentEditable elements with respect to the innerText”.

My current method (partially adapted from this answer: https://stackoverflow.com/a/29258657) of cloning the element, inserting the null character, determining the index and then removing the null character seems to work fine on 99% of websites, but breaks Twitter. I am guessing that Twitter does not like the null character, but I tried with other nonprinting characters and had the same issue. Here is the relevant code (the issue is lines 69-72): https://github.com/rugk/unicodify/blob/09d806b3f731721236b774673d2b2f3db5a40d3f/src/content_scripts/autocorrect.js#L62-L74 We either need another method to determine the caret position or we need a fix for the above code so that it is compatible with Twitter.

rugk commented 3 years ago

Uff, crazy stuff indeed. Here are also some elaborate answers that seem to tacke the same issue… hmm… From testing the fiddle this example here at the top/the updated one looks best…

tdulcet commented 3 years ago

Uff, crazy stuff indeed. Here are also some elaborate answers that seem to tacke the same issue… hmm… From testing the fiddle this example here at the top/the updated one looks best…

This problem with that answer and most of the other solutions is that they provide the caret position with respect to the textContent and for the autocorrect to work correctly, we need it with respect to the innerText. The textContent can include spaces, tabs and other white space characters that do not actually appear when the text is rendered, which breaks the autocorrect feature. See https://github.com/rugk/unicodify/issues/4#issuecomment-831841205 for more information.

rugk commented 3 years ago

Okay, well… you know the details better than me… so this sounds like a very good case for a new Stackoverflow question, when you're linking to these other questions and explain why they don't apply/solve the problem.

So I would kindly ask you to write that question tetx (I guess you can copy much of your explanations :wink:), so I'm assigning that to you here…

tdulcet commented 3 years ago

So I would kindly ask you to write that question tetx (I guess you can copy much of your explanations 😉), so I'm assigning that to you here…

Umm, I have never written a Stack Overflow question, but I can give it a try… Feel free to change anything.

Title: How to get the caret position in ContentEditable elements with respect to the innerText?

Question: I am aware that there are a lot of existing questions about getting the caret position in ContentEditable elements. Almost all of those existing solutions provide the caret position with respect to the textContent. Some examples are this one or this one.

We are currently developing two WebExtensions to do autocorrection as the user types. For example, if the user types :), it could autocorrect it to 😀. For the autocorrection to work, it needs to get the caret position with respect to the innerText. The textContent can include whitespace characters and other differences that do not actually appear when the text is rendered, which breaks the autocorrect feature.

Our current method is partially adapted from this answer: https://stackoverflow.com/a/29258657), which provides the caret position with respect to the innerHTML. It clones the element, inserts the null character, determines the index and then removes the null character:

// document.designMode is handled the same, see  https://github.com/rugk/unicodify/issues/54
if (target.isContentEditable || document.designMode === "on") { 
     target.focus(); 
     const _range = document.getSelection().getRangeAt(0); 
     if (!_range.collapsed) { 
         return null; 
     } 
     const range = _range.cloneRange(); 
     const temp = document.createTextNode("\0"); 
     range.insertNode(temp); 
     const caretposition = target.innerText.indexOf("\0"); 
     temp.parentNode.removeChild(temp); 
     return caretposition; 
}

See our original source code for the full context. This method seems to work fine on 99% of websites, but breaks on Twitter. The cursor is constantly reset to the beginning of the line as the user is typing, which scrambles the text (see the corresponding issue for more information). We are guessing that Twitter does not like the null character, but we tried with other nonprinting characters and had the same issue.

We are looking for another method to determine the caret position with respect to the innerText that will work on all websites, including on Twitter. It needs to support recent versions of both Firefox and Chrome, including Firefox ESR. It also needs to be performant, since it runs on every keypress.

rugk commented 3 years ago

@tdulcet Sounds good, I've changed some stuff such as linking the other Stackoverflow question you're referring to (please double-check that these are the correct ones), using a real question as the title, improved the text used for links and finally added a little off-topic explanation what that designMode is about…

Generally what Stackoverflow people like is if you have a full minimum reproducible example, e.g. if you could create a JSFiddle to demonstrate the issue. Given that this only happens on/in a combination with Twitter though and we are not really sure about what is causing it, I guess we cannot really provide that.

So feel free to submit it! :slightly_smiling_face:

Also, off-topic, but IMHO it would be good if we link all SO or similar questions in the code (in comments) when you use them, as they can give important context and so people can more easily get what/why the code is doing what it does…

tdulcet commented 3 years ago

Generally what Stackoverflow people like is if you have a full minimum reproducible example, e.g. if you could create a JSFiddle to demonstrate the issue. Given that this only happens on/in a combination with Twitter though and we are not really sure about what is causing it, I guess we cannot really provide that.

I still have my demo of the autocorrect feature from https://github.com/rugk/awesome-emoji-picker/issues/66: https://tealdulcet.com/autocorrect/, although as you said, that does not demonstrate the Twitter issue. People could always install the add-on from AMO to see the issue.

So feel free to submit it! 🙂

Oh, I do not have a Stack Overflow account, so feel free to post it yourself if you want… 😉

Also, off-topic, but IMHO it would be good if we link all SO or similar questions in the code (in comments) when you use them, as they can give important context and so people can more easily get what/why the code is doing what it does…

OK, I will keep that in mind… The code in question here was heavily adapted from several sources. I usually only add links if I directly copy something.

[ @tdulcet is this the same reason for the Google Docs breackage? If so, I'd include it here ]

No, Google is messing with the events, which is causing the Google Docs breakage.

rugk commented 3 years ago

Oh, I do not have a Stack Overflow account, so feel free to post it yourself if you want… :wink:

Oh okay, thanks, done: https://stackoverflow.com/q/68822587/5008962

Feel free to follow it… although without an account that could get problematic. There is an RSS feed though :slightly_smiling_face:

rugk commented 2 years ago

Okay, this did not got us far, so I've opened a small bounty of 50 reputation points. I guess the issue is described in a good way, so the only reason can only be people did not look into it/investigated it. Though, I admit it is a pesky issue and only happens on some sites (respectively Twitter), so well…

tdulcet commented 2 years ago

@rugk - Thanks for doing that!

We did get one answer today, but it does not work… If you would like to test it, add this function which implements their answer to our autocorrect.js content script:

function agetCaretPosition(target) {
    // ContentEditable elements
    if (target.isContentEditable || document.designMode === "on") {
        target.focus();
        const selection = document.getSelection();
        const range = selection.getRangeAt(0);
        if (!range.collapsed) {
            return null;
        }
        return selection.focusOffset;
    }
    // input and textarea fields
    else {
        if (target.selectionStart !== target.selectionEnd) {
            return null;
        }
        return target.selectionStart;
    }
}

Then, after this line add this:

    const acaretposition = agetCaretPosition(target);
    if (caretposition !== acaretposition)
        console.error("Expected", caretposition, "Got", acaretposition);

Lastly, open the console on our test site (https://rugk.github.io/unicodify/) and start typing in the “ContentEditable Div”. You will get an error on almost every key press, indicating the carrot position is incorrect. Their answer seems to be attempting to mask the problem, rather than finding a solution.

rugk commented 2 years ago

@tdulcet Thanks a lot for testing, I was just going to ask you to do this. As for the changes, I trust you that you tested it and will comment that it does not work. Otherwise, feel free to also just create a stale/test PR in such a case and directly close it. That's likely the easiest way to reproduce such an issue.

PS: carrot position is a sweet term… :carrot: :blush: :upside_down_face:

rugk commented 2 years ago

I tried to adapt their Fiddle to our example, but that seems to work fine, at least i see no problem here, so I wonder what is the issue: https://jsfiddle.net/d2u65max/1/

tdulcet commented 2 years ago

I tried to adapt their Fiddle to our example, but that seems to work fine, at least i see no problem here, so I wonder what is the issue: https://jsfiddle.net/d2u65max/1/

Here is a Fiddle that demonstrates the issue: https://jsfiddle.net/tcomh4zb/. I can create a PR too if that would be helpful...

rugk commented 2 years ago

Thanks a lot! I guess that is enough for now, I'll forward it to Stackoverfllow.

rugk commented 2 years ago

The SO author adjusted their implementation for bold/italic tags and I tested that in a fiddle https://jsfiddle.net/3mskc146/ again and it now seems to break for br tags. Again also of course forwarded it now.

tdulcet commented 2 years ago

I created a draft PR with the changes from their updated answer: #69. It now seems to return the correct carrot position, but it still does not work on Twitter. 😕

rugk commented 2 years ago

Oh I am still assigned here. So I am of ideas here. Should we merge your PR #69 as it seems to improve the situation? Or does not it help anyway, so we keep the old implementation?

So the only thing we can, apparently, do is what is stated at the beginning in the todo item:

add a hardcoded exclusion list linking the Stackoverflow question and the issue here, if it makes the site totally unusable

Happy new year btw…

tdulcet commented 2 years ago

So I am of ideas here.

I am not sure what to do either, but here are some options I can think of:

Should we merge your PR #69 as it seems to improve the situation? Or does not it help anyway, so we keep the old implementation?

69 does not seem to help, it was only to make it easier to test that Stack Overflow answer on Twitter and reproduce the issue.

So the only thing we can, apparently, do is what is stated at the beginning in the todo item:

I no longer think this would be acceptable, as the issue breaks Twitter, Facebook, Discord and likely other major websites.

Happy new year btw…

Happy New Year to you too! 🎉

rugk commented 2 years ago

I no longer think this would be acceptable, as the issue breaks Twitter, Facebook, Discord and likely other major websites.

Waaait, so many? I was only aware of Twitter so far. But off, that is bad indeed.

Try asking the same question on Mozilla's Discourse (see top of this issue).

Oh damn, I missed that. We should have done it directly when the bounty was active. Anyway did it now here.

Open another bounty of reputation points on your Stack Overflow question and hope we get an acceptable answer this time.

This would be a minimum of 100 points and I'm a little reluctant fo wasting more karma on this, as I doubt we would get a better answer there then… :thinking: Also the comment is fine, but did not help a lot…

tdulcet commented 2 years ago

Waaait, so many? I was only aware of Twitter so far. But off, that is bad indeed.

For Facebook, see #64. Discord is less severe, but also has issues.

Anyway did it now here.

Thanks for doing that! Hopefully we will finally get a solution to this issue… 🤞

rugk commented 2 years ago

Done/Documented in the new FAQ: https://github.com/rugk/unicodify/wiki/FAQ