defmethodinc / just-not-sorry

Chrome extension that warns you when you write emails using words which undermine your message
https://justnotsorry.com/
Other
243 stars 37 forks source link

Highlighting issues on Outlook and GMail #117

Closed sbrudz closed 3 years ago

sbrudz commented 3 years ago

Tami tested the v2.0.0-beta.7 release and found issues with the highlighting on both Outlook and GMail.

From Tami:

Outlook isn't working right… not everything gets caught but I'm not sure why because some were copy and pasted and fine and others were not and the same goes for typed fresh and new. jns-error-1 For gmail I had a similar experience jsn-error-2 I thought the we/i believe/feel worked, but it seems the We's are missing. Also weirdly when I added a space after the try that doesn't trigger, both it and does that make sense triggered. Here's the copy:

sorry i think that just sucks
we believe that trying to actually 
try
does that make sense ?
i believe 
we feel 
i feel 
try 
github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 2.0.0-beta.8 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

sbrudz commented 3 years ago

In testing this fix with Tami, we found that it introduced some new bugs. For example: image The new, looser rules are picking up "I think" and highlighting it. Phrases like "Tami feels" and "Our alumni believe" will also get highlights.

sbrudz commented 3 years ago

Here are some additional details for future reference.

The situation is that when the email DOM text looks like this:

<div>
  just checking
  <div>
    just
  </div>
</div>

Both "just" instances should be highlighted, but the bug is that only the first one is being highlighted.

I tracked it down to this line in util.js: https://github.com/defmethodinc/just-not-sorry/blob/beta/src/helpers/util.js#L27 When we call el.textContent, the resulting string is "just checkingjust" and the regular expression match for just, \b(just)\b doesn't pick up the second one.

I tried using el.innerText instead, which returns "just checking\njust" The match function now picks up both instances, which is good. But the highlight is now misaligned because the \n line break adds another character, which isn't actually in the DOM. I added some logic to compensate for this, but found that textContent handles text nodes with spaces at the end slightly differently than innerText does, causing misalignments that I'm not sure how to compensate for.

I've tried several other options, such as using a TreeWalker, but nothing so far has handled all of the different cases.

sbrudz commented 3 years ago

I've created PR #129 to revert the changes in #124 that loosened the logic too much.

The currently released version of JustNotSorry (1.6.3) has this same issue. Given the difficulty of fixing the bug and the low impact of the problem, I recommend that we stop work on this and focus on other priorities. @tamireiss Are you okay with this course of action?

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 2.1.0-beta.1 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 2.1.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: