Issue #196 - Remove old FF workaround setTimeout #199
Conversation
There was a problem hiding this comment.
Hi @cornu-ammonis,
thanks for your first contribution to this project! 🎉 👍 🏅
I hope you'll like this project and enjoy hacking on it… 😃
So:
- generally, it all seems to work, indeed, at least the text selection
- the "scroll to the top" does not really work as it seems. However, i also saw it did not really work before that PR here…
This generally seems to be a little fragile though…
What this seems to break though is:
- when select text and use the context menu "create QR code" it now then does not select all the text anymore
Note that this happens likely because of that hard timings that happen when the UI is opened and several promises that race against each other. Stuff in InitQrCode…
But I guess I'll have to tackle this anyway, somehow… so I'll certainly have a look here.
BTW in order to test scrolling to top, just use a long URL with some # at the end, e.g. https://longextendedsubdomainnamewithoutdashesinordertotestwordwrapping.badssl.com/#sdfhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhaklshdiowhaodew902123423423423432erasd
| @@ -184,13 +174,7 @@ function scrollToTop(event) { | |||
| return; | |||
| } | |||
|
|
|||
| // Attention: make sure this does not collide with the retry-property set | |||
| // in selectAllText()! | |||
| event.setScrolled = true; | |||
There was a problem hiding this comment.
If we don't even re-try scrolling to the top, I guess we can remove all that setScrolled logic too, can't we?
There was a problem hiding this comment.
That's a good question and I am not sure. Is this still needed? Possibly not…?
Or possibly it is, considering "scroll to top" does not work anyway always in the current situation in my testing at least.
There was a problem hiding this comment.
@rugk yeah, seems like that may call for a larger refactor
There was a problem hiding this comment.
Okay, fine, accepted. Also opened a different issue for that now: #203
This comment has been minimized.
This comment has been minimized.
So it also runs the selection code now when the text at initialisation is overwritten by a context menu message.
|
BTW you can (automatically) let issues close when a PR is merged with the right keyword (i.e. remove "issue" in your PR description). |
Ah my bad, will do! Excited to contribute! |
This likely does not make things better, because when the QR code changes it's size, this also affects the scroll position of the whole textarea. So we better do it at the end, as we did before.
rugk
force-pushed
the
remove-old-ff-workaround
branch
from
bf6a7b9
to
5041d0a
Compare
There was a problem hiding this comment.
So I've also changed some things to fix that the selection was not properly triggered when you use the context menu in the add-on.
Also another issue:
When you click inside of the input field, it does not always select the whole text automatically:
There is an event listener that should force it to select all the text then: https://github.com/rugk/offline-qr-code/pull/199/files#diff-817dd83d26bd35283a14722780bb8481R516
Again, it seems this issue is present in the current master, anyway, so it may not be solvable in this PR.
Hmm yeah, does seem like some race stuff. I agree probably out of scope for this PR, but happy to file another issue and see if I can figure it out! Thanks for the comments / additions here! |
removed! |
Done: #202 |
|
BTW, currently there is the new Hacktoberfest 2019 running. Any PR there counts (if it is made in October). 😃 You may also have a look at all issues tagged with "hacktoberfest" in this repo or somewhere else. |
Fixes #196
Confirmed via testing that this does not change add-on behavior. Also ran all tests. Bugzilla seems to confirm that the target of this workaround was resolved with FF 60.