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

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

Closed
smistad opened this issue Apr 19, 2021 · 18 comments · May be fixed by #213
Closed

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

smistad opened this issue Apr 19, 2021 · 18 comments · May be fixed by #213

Comments

@smistad
Copy link

smistad commented Apr 19, 2021

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

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

@crisluengo
Copy link
Contributor

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

smistad commented Apr 21, 2021

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 mosra added this to TODO in Doxygen theme via automation Jan 5, 2022
@mosra mosra added this to TODO in Python theme via automation Jan 5, 2022
@mosra
Copy link
Owner

mosra commented Jan 5, 2022

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

  • go to https://doc.magnum.graphics/
  • type magnu, m will be autocompleted and highlighted in blue
  • press backspace (or the equivalent on a phone keyboard) -- on desktop browsers it'll erase the m but on mobile the m will stay there, unhighlighted, with cursor before (and thus new text inserted before)
  • if you don't press backspace but continue typing, it'll work as expected (adding m will replace the autocompletion, adding s instead will show no results)

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

smistad commented Jan 5, 2022

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

mosra commented Jan 5, 2022

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:

/* Looks like the user is inserting some text (and not cutting,
copying or whatever), allow autocompletion for the new
character. The oninput event resets this back to false, so this
basically whitelists only keyboard input, including Shift-key
and special chars using right Alt (or equivalent on Mac), but
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)) {
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
oninput), a Backspace after would still result in
autocompleteNextInputEvent, because nothing reset it back. */
} else {
Search.autocompleteNextInputEvent = false;
}

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

mosra commented Jan 5, 2022

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

smistad commented Jan 5, 2022

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

@mosra
Copy link
Owner

mosra commented Jan 5, 2022

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

smistad commented Jan 5, 2022

I use gboard as well..

@mosra
Copy link
Owner

mosra commented Jan 5, 2022

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

mosra commented Jan 5, 2022

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

smistad commented Jan 5, 2022

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

mosra commented Jan 5, 2022

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 👍

@mosra
Copy link
Owner

mosra commented Jan 5, 2022

What a mess.

  • Firefox has the same bug as Chrome with the useless key events, only that it reports Process instead of Unidentified. I thus suspect this is just how Android keyboards work.
  • Programmatically updating <input> value doesn't trigger inputEvent on Chrome or on Firefox desktop (which is reasonable), but does so on Firefox Mobile, so I have to ignore the immediately next inputEvent to fix the "duplicated autocompletion" issue.
  • Deleting the autocompleted text, however, triggers three inputEvents on Firefox:
    1. deleteContentBackward, as expected, with only the unselected text remaining (same as on Chrome)
    2. deleteContentBackward, making the input empty deleting the whole last word (!!)
    3. insertCompositionText, setting the value back to what was there in (1), which of course triggers autocompletion again, resulting in an endless cycle where I just can not delete the highlighted part at all, however it's still highlighted at least, not the issue that was there originally where backspace would just remove the highlight but keep the text

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

mosra commented Jan 5, 2022

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

smistad commented Jan 6, 2022

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

mosra commented Jan 6, 2022

Thanks for the confirmation! I merged the patch as b598223, 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 😆 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? 😅

@mosra mosra closed this as completed Jan 6, 2022
Doxygen theme automation moved this from TODO to Done Jan 6, 2022
Python theme automation moved this from TODO to Done Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Doxygen theme
  
Done
Python theme
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants