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

Document/ask for problematic site-issues #62

Closed
1 of 2 tasks
rugk opened this issue Aug 14, 2021 · 21 comments
Closed
1 of 2 tasks

Document/ask for problematic site-issues #62

rugk opened this issue Aug 14, 2021 · 21 comments
Assignees

Comments

@rugk
Copy link
Owner

rugk commented Aug 14, 2021

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.

#4 (comment)

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

  • write Stackoverflow or so questions, as I e.g. did here, there are a lot of tags for WebExtensions
  • add a hardcoded exclusion list linking the Stackoverflow question and the issue here, if it makes the site totally unusable
@tdulcet
Copy link
Collaborator

tdulcet commented Aug 14, 2021

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):

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;
}

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
Copy link
Owner Author

rugk commented Aug 14, 2021

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
Copy link
Collaborator

tdulcet commented Aug 15, 2021

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 #4 (comment) for more information.

@rugk
Copy link
Owner Author

rugk commented Aug 15, 2021

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 😉), so I'm assigning that to you here…

@tdulcet
Copy link
Collaborator

tdulcet commented Aug 16, 2021

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
Copy link
Owner Author

rugk commented Aug 16, 2021

@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! 🙂

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
Copy link
Collaborator

tdulcet commented Aug 17, 2021

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 rugk/awesome-emoji-picker#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
Copy link
Owner Author

rugk commented Aug 17, 2021

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

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 🙂

@rugk
Copy link
Owner Author

rugk commented Sep 8, 2021

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
Copy link
Collaborator

tdulcet commented Sep 10, 2021

@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
Copy link
Owner Author

rugk commented Sep 10, 2021

@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… 🥕 😊 🙃

@rugk
Copy link
Owner Author

rugk commented Sep 10, 2021

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
Copy link
Collaborator

tdulcet commented Sep 10, 2021

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
Copy link
Owner Author

rugk commented Sep 11, 2021

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

@rugk
Copy link
Owner Author

rugk commented Sep 11, 2021

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
Copy link
Collaborator

tdulcet commented Sep 15, 2021

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
Copy link
Owner Author

rugk commented Jan 2, 2022

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
Copy link
Collaborator

tdulcet commented Jan 3, 2022

So I am of ideas here.

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

  • Open another bounty of reputation points on your Stack Overflow question and hope we get an acceptable answer this time.
    • You did get a comment on the existing answer (see here).
  • Try asking the same question on Mozilla's Discourse (see top of this issue).
  • Disable the Unicode autocorrection feature in Firefox/Chrome until we can find a solution to this, so we can release v1.0.

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
Copy link
Owner Author

rugk commented Jan 3, 2022

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… 🤔
Also the comment is fine, but did not help a lot…

@tdulcet
Copy link
Collaborator

tdulcet commented Jan 4, 2022

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
Copy link
Owner Author

rugk commented Aug 22, 2022

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

@rugk rugk closed this as completed Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants