Skip to content

change(web): drops old IE-related handling code#6557

Merged
jahorton merged 26 commits intomasterfrom
change/web/5943-ie-rooting
Jun 24, 2022
Merged

change(web): drops old IE-related handling code#6557
jahorton merged 26 commits intomasterfrom
change/web/5943-ie-rooting

Conversation

@jahorton
Copy link
Copy Markdown
Contributor

@jahorton jahorton commented Apr 22, 2022

Fixes #5943.
Also addresses some concerns of #3796. As of commit 474634a,

image

While this should be safe, removing numerous old internal hooks like this could have unforeseen complications.

These changes are 16.0-only; they will not land in 15.0.

This PR seeks to remove a lot of the older parts of KMW that were devoted to IE-wrangling. We haven't actively tested KMW on IE in a while and we officially dropped support versions ago. Now, it's time to drop all pretenses of IE support entirely.

That said, we still do need to keep some polyfills around, as older Android devices that we still support launched with old enough versions of Chrome that parts of KMW will instantly break on them unless their users actually update Chrome. I added documenting comments to the most relevant file:

### NOTE ###
# Android API 21 (our current minimum) released with Chrome for Android 37.
# It's also updatable as of this version, but we can't guarantee that the user
# actually updated it, especially on first launch of the Android app/keyboard.
# This one's a minimal, targeted polyfill. es6-shim could do the same,
# but also adds a lot more code the worker doesn't need to use.
# Recommended by MDN while keeping the worker lean and efficient.
cat "../../../node_modules/string.prototype.codepointat/codepointat.js" # Needed for Android / Chromium browser pre-41.
# These two are straight from MDN - I didn't find any NPM ones that don't
# use the node `require` statement for the second. They're also relatively
# short and simple, which is good.
cat "src/polyfills/array.fill.js" # Needed for Android / Chromium browser pre-45.
cat "src/polyfills/array.from.js" # Needed for Android / Chromium browser pre-45.
# For Object.values, for iteration over object-based associate arrays.
cat "src/polyfills/object.values.js" # Needed for Android / Chromium browser pre-54.
# Needed to support Symbol.iterator, as used by the correction algorithm.
cat "src/polyfills/symbol-es6.min.js" # Needed for Android / Chromium browser pre-43.
# Needed to 'support' String.normalize within iOS 9.
# For our limited use case thereof; is definitely NOT a general polyfill.
# (The file size on a true one would be quite high.)
# TBD: should we drop this, since we only support iOS 12+ for the mobile app?
cat "src/polyfills/string.normalize.js"

This is the LMLayer's worker polyfill set. Note that some of these polyfills are used explicitly because we don't want es6-shim bloat in the WebWorker - KeymanWeb itself would use es6-shim instead for such cases. That said, KMW doesn't actually seem to utilize the features being polyfilled... here.

Also, now that we're dropping IE... we can actually just straight-up stop bundling es6-shim into KMW.

User Testing

Test Suite 1: in-app uses

SUITE_IN_APP:

Testing Environments

GROUP_ANDROID_5_APP: Android 5.0 / API 21 (any matching device; emulation is fine)

  • Do NOT update Chrome if requested.
  • If not requested and in an emulated environment, please reset the device to its "original settings" if possible.

GROUP_IOS_12_APP: iOS 12.0 (any matching device; Simulator is fine)

For both, please:

  • Have sil_euro_latin (the default keyboard) installed with predictive text active.
  • Have any other keyboard installed for at least one language.

Tests

  • TEST_INAPP_BASE: Using English(EuroLatin SIL)...
    In portrait orientation, verify OSK is visible and fills the width the bottom of the screen

    1. Verify long-press q key works
    2. Verify long-press p key works
    3. Verify uppercase layer can be selected via SHIFT
    4. Verify number layer can be selected via 123
    5. Verify long-press 1 key works
    6. Verify long-press 0 key works
    7. Verify backspace, space, and enter keys work
    8. Verify Khmer -> Khmer Angkor keyboard can be added via Settings menu
  • TEST_INAPP_LONG_PRESS: Using English(EuroLatin SIL)...

    Type on the OSK using the following scenarios and verify expected output:

    1. Clicking a suggestion on the suggestion banner - should insert the suggestion
    2. short-press a key and release - should insert the base key
    3. long-press a key, select a long-press key, and release - should insert the long-press key
  • TEST_KEYBOARD_SWAP: Use the keyboard's globe key to swap keyboards.

    • If any "Fatal Error" appears or the selected keyboard fails to load properly, FAIL this test.
  • TEST_PREDICTIVE_TEXT: Using English(EuroLatin SIL)...

    1. Use the app's "Clear Text" command to erase all text.
    2. The usual default suggestions should appear: "the", "to", "a".
    3. Type th. All suggestions should begin with these two letters.
    4. Select one of the suggestions. It should replace the th with your selection.

Test Suite 2: standalone use on mobile platforms

SUITE_BROWSER:

Testing Environments

GROUP_ANDROID_6_CHROME: Chrome on Android 6.0 / API 23 (any matching device; emulation is fine)

  • Note: not 5.0. We do extra things in the app to support it that are no longer done for standalone-Web.
    • In particular, note the comments below regarding for ... of and Map.
      GROUP_IOS_12_SAFARI: Safari on iOS 12.0 (any matching device; Simulator is fine)

For both, please:

  • Have sil_euro_latin (the default keyboard) installed with predictive text active.
  • Have any other keyboard installed for at least one language.

Tests

  • TEST_INAPP_BASE: Use the testing page "Test unminified Keymanweb".
    In portrait orientation, verify OSK is visible and fills the width the bottom of the screen

    1. Verify long-press q key works
    2. Verify long-press p key works
    3. Verify uppercase layer can be selected via SHIFT
    4. Verify number layer can be selected via 123
    5. Verify long-press 1 key works
    6. Verify long-press 0 key works
    7. Verify backspace, space, and enter keys work
    8. Verify Khmer -> Khmer Angkor keyboard can be added via Settings menu
  • TEST_LOREM_IPSUM: Use the testing page "Tests performance of touch-alias elements with large amounts of text" - it should be the last one listed on the testing index page.

    • Touch and drag the block of text, then tap somewhere within the text. The caret should be placed near the location of the tap after a very small delay. A bit of lagginess is fine, but it should still feel reasonably responsive.
    • Repeat this at least three times.
    • If the delay ever goes longer than one second, FAIL this test.
    • If the caret is not placed properly at any point, FAIL this test.
    • After the final caret placement, type lorem ipsum. Text entry should feel responsive and no laggier than the other interactions required by this test.

@jahorton jahorton added this to the A16S1 milestone Apr 22, 2022
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Apr 22, 2022
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Apr 22, 2022

User Test Results

Test specification and instructions

✅ SUITE_IN_APP:

8 tests in 2 groups PASSED
  • ✅ GROUP_ANDROID_5_APP: Android 5.0 / API 21 (any matching device; emulation is fine)

    4 tests PASSED
    • TEST_INAPP_BASE (PASSED): Tested Keyman 16.0.12-alpha in Android 5.0 (API 21) emulator as per the instructions and it is working as expected.
    • TEST_INAPP_LONG_PRESS (PASSED): Tested Keyman 16.0.12-alpha in Android 5.0 (API 21) emulator as per the instructions and it is working as expected.
    • TEST_KEYBOARD_SWAP (PASSED): Tested Keyman 16.0.12-alpha in Android 5.0 (API 21) emulator as per the instructions and it is working as expected. (ie., no Fatal error occurs during swapping keyboards)
    • TEST_PREDICTIVE_TEXT (PASSED): Tested Keyman 16.0.12-alpha in Android 5.0 (API 21) emulator as per the instructions and it is working as expected.
  • ✅ GROUP_IOS_12_APP: iOS 12.0 (any matching device; Simulator is fine)

    4 tests PASSED
    • TEST_INAPP_BASE (PASSED): Tested Keyman 16.0.12-alpha build in iOS 12.4 / iPhone 8 Simulator, as per the instructions it is working as expected. (notes)
    • TEST_INAPP_LONG_PRESS (PASSED): Tested Keyman 16.0.12-alpha build in iOS 12.4 / iPhone 8 Simulator, as per the instructions it is working as expected.
    • TEST_KEYBOARD_SWAP (PASSED): Tested Keyman 16.0.12-alpha build in iOS 12.4 / iPhone 8 Simulator, as per the instructions it is working as expected.
    • TEST_PREDICTIVE_TEXT (PASSED): Tested Keyman 16.0.12-alpha build in iOS 12.4 / iPhone 8 Simulator, as per the instructions it is working as expected. (notes)

✅ SUITE_BROWSER:

4 tests in 2 groups PASSED
  • ✅ GROUP_ANDROID_6_CHROME: Chrome on Android 6.0 / API 23 (any matching device; emulation is fine)

    2 tests PASSED
    • TEST_INAPP_BASE (PASSED): Tested this on the corresponding Web page in Android 6.0 / API 23 emulator and it seems to be working as expected. Also, able to install Khmer Angkor. (notes)
    • TEST_LOREM_IPSUM (PASSED): Tested in Chrome (version 68.0) on Android 6.0 / API 23 emulator as per the instructions and it seems to be working as expected. (notes)
  • ✅ GROUP_IOS_12_SAFARI: Safari on iOS 12.0 (any matching device; Simulator is fine)

    2 tests PASSED
    • TEST_INAPP_BASE (PASSED): Tested this as per the instructions in iOS 12.4 / iPhone 8 Simulator and it is working as expected. Also, able to install Khmer Keyboard. (notes)
    • TEST_LOREM_IPSUM (PASSED): Tested in Safari Browser on iOS 12.4 / iPhone 8 Simulator and it seems to be working as expected. ie., Able to see the caret appaers near the location of the tap. After the final caret placement, I am able to type 'lorem ipsum' without any lagging. (notes)

Test Artifacts

@jahorton
Copy link
Copy Markdown
Contributor Author

Ooh, a "fun" note: when testing on an un-updated Android API 21 (5.0) image, most KMW testing pages will break!

To fix this, replace web/testing/commonHeader.js's version of this function with this one:

  function errToString(err) {
    // Painful?  Kinda.  But needed on un-updated Android API 21!
    if(Array.isArray(err)) {
      var result = '';
      for(var i = 0; i < err.length; i++) {
        var e = err[i];
        if(e.error instanceof Error) {
          result += e.error.message + '\n';
        } else {
          result += JSON.stringify(e) + '\n';
        }
      }
      return result;
    }
    if(err instanceof Error) {
      return err.message;
    }
    return JSON.stringify(err);
  }

Neither let nor for...of is supported in the unupdated version of its browser!

@jahorton
Copy link
Copy Markdown
Contributor Author

jahorton commented Apr 22, 2022

To test the previous commit (f14e7f1), I dummied out the PromiseStore Map dependence issue noted in this comment on the base issue: #5943 (comment)

After doing so, the standard unminified test page appeared fully functional on an emulated API 21 device running Chrome for Android 37. Therefore, the one remaining reason to continue using es6-shim is for its Map polyfill.

Noting the significant file size it brings, that leaves us with a choice:

  • Do we leave things as they are?
  • Do we custom write a limited polyfill - or just rewrite PromiseMap - to eliminate the dependency?
    • Allows us to maintain API 21 compat, even when Chrome isn't updated
  • Or, do we just drop support for significantly older versions of Chrome for Android?
    • Note: will cause keyboard breakage in the Android app until the user updates Chrome.
    • Well... unless we have Android itself include es6-shim and insert it into the keyboard host page if the Chrome WebView isn't sufficiently updated. (Won't fix unembedded Web, but it'll at least prevent the app from breaking.)

The latter two options allow us to drop es6-shim and thus significantly reduce KMW's file size.

@jahorton
Copy link
Copy Markdown
Contributor Author

Note tagged PRs - there are a few comments noted here that motivate use of arrow functions, which had always been an issue for IE. Adjustments will be possible once beta is merged into master near the end of sprint.

@jahorton
Copy link
Copy Markdown
Contributor Author

Extra note: make absolutely sure that IE has been removed from all test configs for Web - such as in the "manual" (local) unit-testing configuration on Windows.

@mcdurdin mcdurdin modified the milestones: A16S2, A16S3 May 28, 2022
@bharanidharanj
Copy link
Copy Markdown

SUITE_IN_APP:

GROUP_IOS_12_APP: iOS 12.0 (any matching device; Simulator is fine)

  • TEST_INAPP_BASE (PASSED): Tested Keyman 15.0.260-alpha build in iPhone 13 Pro Simulator, as per the instructions it is working as expected.
  • TEST_INAPP_LONG_PRESS (PASSED): Tested Keyman 15.0.260-alpha build in iPhone 13 Pro Simulator, as per the instructions it is working as expected.
  • TEST_KEYBOARD_SWAP (PASSED): Tested Keyman 15.0.260-alpha build in iPhone 13 Pro Simulator, as per the instructions it is working as expected.
  • TEST_PREDICTIVE_TEXT (PASSED): Tested Keyman 15.0.260-alpha build in iPhone 13 Pro Simulator, as per the instructions it is working as expected.

@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Jun 15, 2022

SUITE_BROWSER:

GROUP_ANDROID_6_CHROME: Chrome on Android 6.0 / API 23 (any matching device; emulation is fine)

  • TEST_INAPP_BASE (PASSED): Tested this on the corresponding Web page in Android 6.0 / API 23 emulator and it seems to be working as expected. Also, able to install Khmer Angkor.

  • TEST_LOREM_IPSUM (FAILED): Not able to test this page since clicking on the testing index page shows 404 error message.

@bharanidharanj
Copy link
Copy Markdown

SUITE_IN_APP:

GROUP_IOS_12_APP: iOS 12.0 (any matching device; Simulator is fine)

  • TEST_INAPP_BASE (PASSED): Tested Keyman 16.0.12-alpha build in iOS 12.4 / iPhone 8 Simulator, as per the instructions it is working as expected.

  • TEST_INAPP_LONG_PRESS (PASSED): Tested Keyman 16.0.12-alpha build in iOS 12.4 / iPhone 8 Simulator, as per the instructions it is working as expected.
  • TEST_KEYBOARD_SWAP (PASSED): Tested Keyman 16.0.12-alpha build in iOS 12.4 / iPhone 8 Simulator, as per the instructions it is working as expected.
  • TEST_PREDICTIVE_TEXT (PASSED): Tested Keyman 16.0.12-alpha build in iOS 12.4 / iPhone 8 Simulator, as per the instructions it is working as expected.

@bharanidharanj
Copy link
Copy Markdown

SUITE_BROWSER:

GROUP_IOS_12_SAFARI: Safari on iOS 12.0 (any matching device; Simulator is fine)

  • TEST_INAPP_BASE (PASSED): Tested this as per the instructions in iOS 12.4 / iPhone 8 Simulator and it is working as expected. Also, able to install Khmer Keyboard.

  • TEST_LOREM_IPSUM (FAILED): Not able to test it since it throwing 404 error message.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Jun 15, 2022
@jahorton
Copy link
Copy Markdown
Contributor Author

jahorton commented Jun 16, 2022

Weird that you're getting a 404 when following that link; it works perfectly fine for me right now. Maybe something went funny with the server when you were testing?

BTW, I believe that in this scenario, that'd be a "BLOCKED", not a "FAILED" - you didn't even get to run the tests, after all.

SUITE_BROWSER

GROUP_ANDROID_6_CHROME
TEST_LOREM_IPSUM OPEN

GROUP_IOS_12_SAFARI
TEST_LOREM_IPSUM OPEN

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Jun 16, 2022
@bharanidharanj
Copy link
Copy Markdown

Weird that you're getting a 404 when following that link; it works perfectly fine for me right now. Maybe something went funny with the server when you were testing?

BTW, I believe that in this scenario, that'd be a "BLOCKED", not a "FAILED" - you didn't even get to run the tests, after all.

SUITE_BROWSER

GROUP_ANDROID_6_CHROME TEST_LOREM_IPSUM OPEN

GROUP_IOS_12_SAFARI TEST_LOREM_IPSUM OPEN

@jahorton Ah..Yes. It is "BLOCKED" and not "FAILED". Thanks Josh, for the clarification. 👍

@mcdurdin
Copy link
Copy Markdown
Member

  • Well... unless we have Android itself include es6-shim and insert it into the keyboard host page if the Chrome WebView isn't sufficiently updated. (Won't fix unembedded Web, but it'll at least prevent the app from breaking.)

This is my preference: Keyman Engine for Web should not include any polyfills. Any app that uses Keyman Engine for Web (whether that be a native device app like Keyman for Android, or a website like keymanweb.com) can decide which versions of browsers it wants to support and provide the necessary polyfills. This is similar to our strategy for supporting error reporting -- it's up to the app, not Keyman Engine for Web, to make these choices.

(The worker may be an issue, admittedly -- any ideas?)

What we should do, to be nice JS citizens, is provide guidance in the Keyman Engine for Web documentation on which features may require polyfills.

Note: if we want keymanweb.com to support older browsers, we can include es6-shim.js there. Analysis of current device users may help on this.

Copy link
Copy Markdown
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First-pass review 😁

// Think of it as performing a floor() function, but the floor depends on the origin's direction.
let snapOrder: (x: number, y: number) => boolean;
const isRTL = (this as unknown as HTMLElement).dir == 'rtl';
const snapOrder = isRTL ? (a, b) => a < b : (a, b) => a > b;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except... they're actually transpiling now?

Yeah, I noticed that they've been transpiling properly for a while. Perhaps we had the wrong output version set at some point?

Comment on lines +309 to +312
e.style.MozUserSelect="none";
e.style.KhtmlUserSelect="none";
e.style.UserSelect="none";
e.style.WebkitUserSelect="none";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three of these are probably also unnecessary now but I haven't checked version support for *UserSelect?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://developer.mozilla.org/en-US/docs/Web/CSS/user-select

The fact that the article still references all of them and their differences is something I find concerning.

@jahorton
Copy link
Copy Markdown
Contributor Author

jahorton commented Jun 16, 2022

(The worker may be an issue, admittedly -- any ideas?)

Always has been. That'd take some time and investment to work around, I think.


Rough approach outline(?):

For inlined scripts: https://stackoverflow.com/a/34854903

For file-based scripts: it sounds like something AJAX-based would be required.

Once all scripts are loaded, they could then be prepended to the worker Blob. Of course, note how this will add more asynch-ness to our keyboards' startup time.

@bharanidharanj
Copy link
Copy Markdown

SUITE_BROWSER:

GROUP_ANDROID_6_CHROME: Chrome on Android 6.0 / API 23 (any matching device; emulation is fine)

  • TEST_LOREM_IPSUM (BLOCKED): Not able to test this issue, since I am getting 404 error message.

@bharanidharanj
Copy link
Copy Markdown

bharanidharanj commented Jun 16, 2022

SUITE_BROWSER:

GROUP_IOS_12_SAFARI: Safari on iOS 12.0 (any matching device; Simulator is fine)

  • TEST_LOREM_IPSUM (BLOCKED): Not able to test this issue, since I am getting 404 error message.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Jun 16, 2022
@mcdurdin
Copy link
Copy Markdown
Member

  • TEST_LOREM_IPSUM (BLOCKED): Not able to test this issue, since I am getting 404 error message.

@bharanidharanj I am able to replicate this error page. We will fix that in an upcoming update to the test pages. However, I am not sure why you need to click that link in order to run the test? You should be able to navigate from the User Test Results artifacts section:

image

to:

image

and from there, to:

image

without ever needing to click the "Return to main index" link? (If you accidentally go to the wrong page, use the back button in your browser)

@keymanapp-test-bot retest TEST_LOREM_IPSUM

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Jun 16, 2022
mcdurdin added a commit that referenced this pull request Jun 16, 2022
We need a '../index.html' link rather than '../' on the three
second-level KeymanWeb test pages, in order to get to the index page for
artifacts when the test pages are hosted on build.palaso.org --
otherwise TeamCity returns a 404. (This is probably also good for
file-system-based navigation.)

We don't need the same reference on other directories, just for the
top-level index.html.

Identified at #6557 (comment)
printf "function %s () {\n" "${name}"

# TODO: I don't know if this is still true, or if we can add the polyfills to
# the project directly now?
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. We could test this, I suppose. It's possible the declaration emitter got enhanced in the newer TS version we're using now.

It definitely fell over quickly when I tried directly linking against it with allowJS=true in the config before we were using incremental builds.

@bharanidharanj
Copy link
Copy Markdown

  • TEST_LOREM_IPSUM (BLOCKED): Not able to test this issue, since I am getting 404 error message.

@bharanidharanj I am able to replicate this error page. We will fix that in an upcoming update to the test pages. However, I am not sure why you need to click that link in order to run the test? You should be able to navigate from the User Test Results artifacts section:

image

to:

image

and from there, to:

image

without ever needing to click the "Return to main index" link? (If you accidentally go to the wrong page, use the back button in your browser)

@keymanapp-test-bot retest TEST_LOREM_IPSUM

@mcdurdin I thought by clicking on the 'Return to main index' link would show the corresponding test page. Anyhow, thanks for your suggestion. I will retest it and post my comment. 👍

@bharanidharanj
Copy link
Copy Markdown

SUITE_BROWSER:

GROUP_IOS_12_SAFARI: Safari on iOS 12.0 (any matching device; Simulator is fine)

  • TEST_LOREM_IPSUM (PASSED): Tested in Safari Browser on iOS 12.4 / iPhone 8 Simulator and it seems to be working as expected. ie., Able to see the caret appaers near the location of the tap. After the final caret placement, I am able to type 'lorem ipsum' without any lagging.

@bharanidharanj
Copy link
Copy Markdown

SUITE_BROWSER:

GROUP_ANDROID_6_CHROME: Chrome on Android 6.0 / API 23 (any matching device; emulation is fine)

  • TEST_LOREM_IPSUM (PASSED): Tested in Chrome (version 68.0) on Android 6.0 / API 23 emulator as per the instructions and it seems to be working as expected.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Jun 17, 2022
Copy link
Copy Markdown
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any change to polyfill strategy (moving responsibility to the consumer of Keyman Engine for Web) belongs in a separate PR, so this LGTM now (two minor comments pending)

@jahorton jahorton merged commit f684133 into master Jun 24, 2022
@jahorton jahorton deleted the change/web/5943-ie-rooting branch June 24, 2022 01:35
@keyman-server
Copy link
Copy Markdown
Collaborator

Changes in this pull request will be available for download in Keyman version 16.0.19-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore(web): remove remaining IE references

4 participants