search

keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
372 stars 102 forks source link

feat(web): re-implementation of introductory globe-key help bubble #7612

Closed jahorton closed 1 year ago

jahorton commented 1 year ago

Fixes #7497.

This implements the old, Android-code-based globe key help bubble as a Web-side, DOM-based help bubble via JS and CSS.

Current implementation:

image

image

For comparison against the old implementation, it's probably best to review this PR in tandem with the removed code from #7473. There are a few notable changes here, though...

  1. isHelpBubbleEnabled, setHelpBubbleEnabled remain deprecated. Looking at #7473... I couldn't discern what role they actually played.
    • Compared against the iOS implementation, I would expect a check against there being multiple installed keyboards or similar... but it didn't exist, so there's nothing in that regard to restore.
  2. This does add getShouldShowHelpBubble and setShouldShowHelpBubble as wrappers for the private property ShouldShowHelpBubble that did exist in #7473.
    • Looking at things overall, there is a fair argument to be made for renaming them to the methods mentioned in point 1 above.
    • getShouldShowHelpBubble facilitates "lazy initialization"; this allows a library consumer to override the engine-default value if desired, before any logical checks against the property are performed.
      • Another aspect of this - we initialize the property once during the keyboard's life cycle.
      • Should the page be reloaded, the help bubble will no longer be redisplayed. (Well, unless the help bubble was never actually dismissed.)
  3. Rather than automatically turning off the "should show" flag and "manually" restarting the bubble after dismissal on cases that should temporarily hide it, we now preserve the "should show" flag and only unset it after a "final" dismissal. The flag is left active during 'temporary hides'.
    • This approach was far simpler to maintain since the help bubble's visualization is on the opposite side of the WebView boundary from the code that wishes to check if it's active, which was the old approach.

A quick note of the desired behaviors I could identify:

Cases that should show the help bubble:

Cases where if it is still displayed, it should be hidden, then redisplayed:

Cases where hiding the help bubble should result in permanent dismissal:

Cases where hiding the help bubble should dismiss the bubble for the lifetime of the keyboard... but not permanent:

User Testing

If the following tests are performed in order, without interruption, a clean install should only be required once, for the first test. Otherwise, a clean install should be performed when resuming tests after a break. (Steps 1-3 of TEST_CLEAN_INSTALL_BUBBLE correspond to such a "clean install.")

Do not directly swap keyboards unless otherwise instructed!

Tests should be carried out on a real Android phone if possible.

NOTE: once TEST_BUBBLE_POST_SWAP is completed, a clean install will be required to re-run any of these tests on the device used for testing!

keymanapp-test-bot[bot] commented 1 year ago

User Test Results

Test specification and instructions

Test Artifacts

jahorton commented 1 year ago

Notes for further development:

https://github.com/keymanapp/keyman/blob/68dc7e8cb9523dca441b5b5772ec8cc02782cbfb/android/KMEA/app/src/main/res/values/strings.xml#L267-L268

https://github.com/keymanapp/keyman/blob/68dc7e8cb9523dca441b5b5772ec8cc02782cbfb/ios/engine/KMEI/KeymanEngine/en.lproj/Localizable.strings#L106-L107

Perusing the various ways this has been localized may provide some "fun" dev-testing material, since the string could end up longer in some languages.

jahorton commented 1 year ago

The original style spec for Android's help bubble:

https://github.com/keymanapp/keyman/blob/68dc7e8cb9523dca441b5b5772ec8cc02782cbfb/android/KMEA/app/src/main/res/layout/help_bubble_layout.xml#L11-L22

jahorton commented 1 year ago

At this point, I've got the Web-side of the globe-key bubble pretty well fleshed out... at least aside from one detail.

Looking through the changes of #7473, there's one function in particular that's being a bit difficult to adapt: the Android-side dismissHelpBubble:

https://github.com/keymanapp/keyman/blob/c25458074774ac1abd29abb0d13922189151d72c/android/KMEA/app/src/main/java/com/tavultesoft/kmea/KMKeyboard.java#L1370-L1382

To be explicit, the difficult parts are lines 1374 and 1376 - the return statements. The ability to note a need to redisplay the help bubble after keyboard changes or keyboard-backgrounding was used to keep the bubble's display to a reasonable minimum for user guidance, rather than constantly spamming it. The issue is... as the help bubble's active status is now managed Web-side, rather than Java-side, I'm not immediately clear on how best to restore that logic due to the complications posed by the WebView barrier.

Some of the use cases could easily be 100% managed on the Web side - restoring the help bubble after device rotation (so that it's properly sized and positioned for the new orientation) is such a case. However, code within the WebView isn't privy to knowledge of when the app / system-keyboard has been backgrounded and then restored - this case requires something extra.

mcdurdin commented 1 year ago

I think we can probably be a bit more straightforward on the help bubble? We just dismiss it on first touch on the globe button, or else, say, 10 seconds after first display.

How do we record that it has been dismissed?

I don't think we ever need to show the bubble again after that first time.

jahorton commented 1 year ago

I think we can probably be a bit more straightforward on the help bubble? We just dismiss it on first touch on the globe button, or else, say, 10 seconds after first display.

How do we record that it has been dismissed?

I don't think we ever need to show the bubble again after that first time.

We don't persist anything that says "we've shown this once before" that'll be available on future loads of the app, as if the process were killed and restarted.

Internally, there is a variable (KMKeyboard.ShouldShowHelpBubble) that is set to true on app load. It's auto-cleared if the corresponding toggle is off, and is also cleared once dismissed. That said, there are a couple of cases where dismissal will automatically re-display the hint:

And...

The problem noted in my previous comment: the "with the hint still active" part.

mcdurdin commented 1 year ago

OK, so I'm not clear why the KMKeyboard.ShouldShowHelpBubble isn't the flag we need?

darcywong00 commented 1 year ago

How do we record that it has been dismissed?

I think it's these removed lines in KMManager.java from #7473 

        SharedPreferences prefs = appContext.getSharedPreferences(appContext.getString(R.string.kma_prefs_name), Context.MODE_PRIVATE);
        SharedPreferences.Editor editor = prefs.edit();
        editor.putBoolean(KMManager.KMKey_ShouldShowHelpBubble, false);
        editor.commit();
jahorton commented 1 year ago

@darcywong00 Revisiting the code removed by #7473...

https://github.com/keymanapp/keyman/blob/c25458074774ac1abd29abb0d13922189151d72c/android/KMEA/app/src/main/java/com/tavultesoft/kmea/KMKeyboard.java#L1339-L1346

(via release-16.0.84-alpha)

What mechanisms would have caused the PopupWindow to self-dismiss? It seems implied that a touch on it may have led to dismissal, but my best reads indicate the opposite - a touch anywhere else but on it would lead to dismissal.

darcywong00 commented 1 year ago

I think a touch on the OSK leads to the globe key dismissal.

jahorton commented 1 year ago

Okay, major update; I believe the basic functionality is now restored and in place. Jury's out on the styling though - there's almost certainly room for improvement there.

The two main parameters to tweak re: bubble shape:

  1. fontSize
    • currently based on KMW's "base" font size for the OSK root. Not doing this tends to result in overly-small text.
  2. width
    • currently set based on / proportional to the globe key's width. We could probably just set a fixed width instead, though.

The bubble's CSS styling will handle all other aspects of the layout once those are set. Unfortunately, getting multiline text with tightly-constrained element width isn't exactly possible without lots of manual, fiddly, and complex logic.

Based on those two settings, the height of the bubble will dynamically increase as needed to wrap its text.

jahorton commented 1 year ago

So... turns out the source files under web/source/osk/embedded were all being included in the non-embedded build product. The latest commit disables this, which should resolve the file-size change warning-check.

Vice-versa as well, except we actually need the browser-oriented parts to be included for embedding in iOS.

darcywong00 commented 1 year ago

Hmm, I'm trying this on Android 5.0 (API 21) and getting our fave console error

Uncaught TypeError: undefined is not a function

Sentry said line 6607 but I'm not seeing a culprit...

                if (transformDistribution && transformDistribution.length > 0) {
                    editedToken.transformDistributions.push(transformDistribution);
                    if (this.searchSpace) {
                        this.searchSpace.forEach(function (space) { return space.addInput(transformDistribution); });
                    }
                }
sentry-io[bot] commented 1 year ago

Sentry issue: KEYMAN-WEB-8A

darcywong00 commented 1 year ago

Maybe the branch is stale and doesn't have the polyfills added from recent PRs?

mcdurdin commented 1 year ago

Sentry said line 6607 but I'm not seeing a culprit...

forEach is Chrome 38+

Except that's set.forEach, not Array.forEach

mcdurdin commented 1 year ago

So... turns out the source files under web/source/osk/embedded were all being included in the non-embedded build product. The latest commit disables this, which should resolve the file-size change warning-check.

Nice, I think the check/web/file-size status check has just paid for itself!

darcywong00 commented 1 year ago

Maybe the branch is stale and doesn't have the polyfills added from recent PRs?

yeah, I merged locally with beta and the console log is now clear

mcdurdin commented 1 year ago

image

3,398 bytes smaller, now 390,858 bytes!

jahorton commented 1 year ago

Maybe the branch is stale and doesn't have the polyfills added from recent PRs?

This. I noticed a number of related omissions when doing the diff checks that led to my discovery of the embedded code's inclusion.

bharanidharanj commented 1 year ago
keyman-server commented 1 year ago

Changes in this pull request will be available for download in Keyman version 16.0.110-beta