mosra / m.css

A no-nonsense, no-JavaScript CSS framework, site and documentation theme for content-oriented websites
https://mcss.mosra.cz
Other
409 stars 92 forks source link

Search.js in doxygen documentation module not working on mobile #196

Closed smistad closed 2 years ago

smistad commented 3 years ago

Tested this on Android with both chrome and firefox. The autocomplete inserts text while typing, making it impossible to search.

It seems to me that the code that marks the autocompleted text to be replaced when the next key is typed, is not working.

I can contribute to a fix, if anyone can point to where in the code the issue might be.

crisluengo commented 3 years ago

Search works fine on iOS with both Safari and Firefox. This is likely an Android bug.

crisluengo commented 3 years ago

I just tried this on a Samsung phone (Android, Chrome) and it works just fine as well.

Autocomplete inserts text, but it is selected, meaning that as you type the selection (inserted text) gets replaced. It works the same way on all platforms and browsers I've tried so far, and is very convenient. Disabling this behavior would be a reduction in usability.

smistad commented 3 years ago

Hm, strange, on my phone the added text is selected, but when I continue to type, the text is inserted before the added text instead of replacing it. I agree that finding a fix is better than removing the functionality.

mosra commented 2 years ago

(Sorry for embarrassingly late replies, finally got a chance to get back to this project.)

Hmm, seems like I can reproduce this, although maybe a bit differently(?):

So I suppose there's something weird in how I implement the autocompletion highlight, causing mobile browsers to not highlight in certain circumstances, which then results in the autocompleted text not being overwritten when typing?

smistad commented 2 years ago

I tried doing the same as you @mosra On my mobile, if I try to type m after m was autocompleted, I get two m's, so magnumm. If I try backspace I get the same issue as you.

So yeah, definitively something weird going on here, which is why I disabled this on mobile for my own documentation page

mosra commented 2 years ago

Eh, stupid Chrome, of course it's the WONTIFX bug that's linked from the comment in the code (https://bugs.chromium.org/p/chromium/issues/detail?id=118639) or maybe some variant of it:

https://github.com/mosra/m.css/blob/fb5fd851bb0d5bd855ffeaaf560a9a8c55297d18/documentation/search.js#L734-L752

All event.keys from the virtual keyboard reporting as Unidentified, meaning I can't check if backspace was pressed or not. What's extremely funny is that if I use remote Chrome Developer Tools, the keys entered from a physical keyboard have the correct event.key (the last entry in the console log):

image

Trying to find another solution for this.

mosra commented 2 years ago

On my mobile, if I try to type m after m was autocompleted, I get two m's, so magnumm.

Huh, but that's different from what I see here. I get magnum (the blue m becomes white), not magnumm...

smistad commented 2 years ago

I get double m's on both firefox and chrome on my mobile

mosra commented 2 years ago

I suspect it's maybe also related to the virtual keyboard you use. Which one do you have? I'm on the vanilla gboard I think.

smistad commented 2 years ago

I use gboard as well..

mosra commented 2 years ago

This patch should disable autocompletion completely on the buggy Android virtual keyboard, do you have an easy way to try it out?

diff --git a/documentation/search.js b/documentation/search.js
index 5346fa63..ce7a7dea 100644
--- a/documentation/search.js
+++ b/documentation/search.js
@@ -739,15 +748,25 @@ if(typeof document !== 'undefined') {
                excluding Ctrl-key, which is usually not for text input. In the
                worst case the autocompletion won't be allowed ever, which is
                much more acceptable behavior than having no ability to disable
-               it and annoying the users. See also this WONTFIX Android bug:
-               https://bugs.chromium.org/p/chromium/issues/detail?id=118639 */
-            } else if(event.key != 'Backspace' && event.key != 'Delete' && !event.metaKey && (!event.ctrlKey || event.altKey)) {
+               it and annoying the users. */
+            } else if(event.key != 'Backspace' && event.key != 'Delete' && !event.metaKey && (!event.ctrlKey || event.altKey)
+                /* Don't ever attempt autocompletion with Android virtual
+                   keyboards, as those report all `event.key`s as
+                   `Unidentified` with `event.code` 229 and thus we have no way
+                   to tell if a text is entered or deleted. See this WONTFIX
+                   bug for details:
+                    https://bugs.chromium.org/p/chromium/issues/detail?id=118639
+                   Another option would be to use inputEvent there and catch
+                   deletion. */
+                && event.key != 'Unidentified'
+            ) {
                 Search.autocompleteNextInputEvent = true;
             /* Otherwise reset the flag, because when the user would press e.g.
                the 'a' key and then e.g. ArrowRight (which doesn't trigger

Meanwhile I'm trying to find a better solution that doesn't just kill the feature altogether.

mosra commented 2 years ago

Okay, I think I have it -- for Chrome at least, if I delay-call setSelectionRange() in a timeout instead of directly, it gets rid of the nasty bugs where the selection was rendered but didn't actually work.

I temporarily uploaded the patched script to https://doc.magnum.graphics/ to make it easier to test -- can you try there with magnu again? It should behave properly now, although you may need to open it in a private tab or something to bypass the cache. EDIT: stable link: https://tmp.magnum.graphics/search-android-fix/ -- type an, should autocomplete to another and work as desired.

Thanks in advance for a confirmation! :)

smistad commented 2 years ago

I tried your tmp url. Now I don't get the duplication issue on chrome, but backspace is still broken. On firefox both errors are still there

mosra commented 2 years ago

Ah well :/ So I guess I'll have to disable this altogether.

This URL has the above patch, disabling autocompletion completely when (buggy) Chrome is detected: https://tmp.magnum.graphics/search-android-nocompletion/ -- typing an should not autocomplete at all. Does that work for you and does that disable autocompletion on Firefox as well?

I finally managed to reproduce both of your issues in Firefox. Will report if I get anywhere, thanks for the help so far :+1:

mosra commented 2 years ago

What a mess.

I'm not sure why it has to delete the whole word, possibly it's some shitty workaround in order to trigger proper text reshaping? ugh

mosra commented 2 years ago

I give up. The amount of suffering is just too much for such a minor feature.

The version at https://tmp.magnum.graphics/search-android-nope-nothing/ should have autocompletion disabled for both Chrome and Firefox on Android. If you can confirm it's indeed the case (so an won't autocomplete to other on neither of the two browsers for you), I'll push this fix to the master branch. Thanks again :)

smistad commented 2 years ago

Thanks for trying anyway. I can confirm that your new version disabled autocomplete on both Chrome and Firefox on my mobile device.

I proposed in this PR https://github.com/mosra/m.css/pull/197 to just disable autocomplete completely for mobile. Maybe that is a better option if this bug relates to the virtual keyboard on mobile devices and not the browsers?

mosra commented 2 years ago

Thanks for the confirmation! I merged the patch as b59822342db1622a74ab4b95b844073ef93a2440, and stashed the unfinished attempt to fix this properly as #213.

Since @crisluengo confirmed this works fine with iOS (and the Safari browser, no matter if skinned as FF or Chrome), I'd keep it working there. It's likely due to how Android implements virtual keyboard, which is reflected in the uselessness of key events. OTOH, if I connect a physical keyboard to Android or use the "remote Chrome" and enter keystrokes from there, there are no problems whatsoever. So I think the way I detect and skip autocompletion is neither too narrow nor too broad.

Thanks for proposing #197 nevertheless. I have to admit the "mobile detection" monstrosity that lists every vendor / device name ever in existence is quite frightening :laughing: How can one possibly be sure it won't have any false positives? Or am I supposed to refresh that regexp after every CES to accomodate for fresh batches of devices? :sweat_smile: