keymanapp / help.keyman.com

https://help.keyman.com/ source
8 stars 29 forks source link

change: use published Keyman Engine API to fetch keyboards in keyboard help pages #1272

Closed jahorton closed 1 month ago

jahorton commented 4 months ago

While fixing https://github.com/keymanapp/keyman/issues/11467, I noticed that some of the font-styling for keyboards wasn't being applied. It turns out that this was due to help.keyman.com bypassing keyboard stubs (Web's equivalent to kmp.json) entirely, thus breaking the link between keyboard and fonts.

In older versions of Keyman Engine for Web, this likely wasn't an issue due to how entangled OSK code was with the rest of the Keyman Engine implementation. Part of preparing the OSK to (eventually) become a standalone module means it needs to rely on data that the OSK, by itself, has access to - and the design for that is reliant upon keyboard stub information.

mcdurdin commented 4 months ago

In older versions of Keyman Engine for Web, this likely wasn't an issue due to how entangled OSK code was with the rest of the Keyman Engine implementation. Part of preparing the OSK to (eventually) become a standalone module means it needs to rely on data that the OSK, by itself, has access to - and the design for that is reliant upon keyboard stub information.

Note that the original design for KeymanWeb supported direct reference to keyboards with script elements rather than stubs. So if this no longer works, we really need to clearly document this as a breaking change. It may impact other users.

jahorton commented 4 months ago

In older versions of Keyman Engine for Web, this likely wasn't an issue due to how entangled OSK code was with the rest of the Keyman Engine implementation. Part of preparing the OSK to (eventually) become a standalone module means it needs to rely on data that the OSK, by itself, has access to - and the design for that is reliant upon keyboard stub information.

Note that the original design for KeymanWeb supported direct reference to keyboards with script elements rather than stubs. So if this no longer works, we really need to clearly document this as a breaking change. It may impact other users.

To the best of my knowledge, pre-loading the keyboards was intended - but there was nigh-always a stub loader paired with it as well. (The stub could be loaded after the keyboard, rather than be what triggered the load.) Even without the loader, the keyboard will still be loaded into the engine - just without font info. For keyboards reliant on super-common fonts, like Tahoma or Arial, this poses little issue if the system already has the font. For more specific fonts, though, it appears to me that the stub's always been the link.

Also, note that a keyboard won't show up on any Web keyboard-picker UI when no matching stub is available. All the UI modules base their pickers off of available stubs - and that's not new to 17.0.

From KMW 10.0 - keyman.getKeyboards() passes through the following logic:

https://github.com/keymanapp/keyman/blob/f16872a99896a2fc208c68ac92a827820ab07e3f/web/source/kmwkeyboards.ts#L153-L156

    getDetailedKeyboards() {
      var Lr = [], Ln, Lstub, Lrn;

      for(Ln=0; Ln < this.keyboardStubs.length; Ln++) { // I1511 - array prototype extended

And that... is a pretty close adaptation from KMW 2.0:

https://github.com/keymanapp/keyman/blob/75864966f480c2773980faefe3d35021165d6d53/web/source/keymanweb.js#L3318-L3322

    keymanweb['getKeyboards'] = function()
    {
      var Lr = [], Ln, Lstub, Lrn;

      for(Ln=0; Ln<keymanweb._KeyboardStubs.length; Ln++)  // I1511 - array prototype extended

Why the focus on keyman.getKeyboards()?

Both use it to build the keyboard-selection list. I can also trace effectively-identical dependencies within the Touch "UI".

mcdurdin commented 4 months ago

To the best of my knowledge, pre-loading the keyboards was intended - but there was nigh-always a stub loader paired with it as well.

Not sure why you are arguing with the person who did the original design.

In the original design, the intent of the keyboard stub was to avoid the network cost of loading a keyboard that wouldn't be needed. The stub was not originally intended to have metadata that the keyboard itself didn't have, and so was optional. (Not saying this is good design, just saying what it was.) At the time of the original design that XMLHttpRequest/AJAX didn't exist. We can do better now of course.

jahorton commented 1 month ago

Double-checking... yep, we're in business. The current stable-17.0 works properly with these changes.

image

Granted, there may be a few layout-formatting issues to tweak.

mcdurdin commented 1 month ago

Granted, there may be a few layout-formatting issues to tweak.

OK, shall we get those in before we merge -- there's no rush to merge this really, so we might as well get it right.

jahorton commented 1 month ago

With 16.0 in place...

image

Differences:

mcdurdin commented 1 month ago
  • 16.0 lacks the keyboard name in spacebar text (as opposed to having it, but at too large a scale)
  • 16.0 does not affect page text, whereas 17.0 does

These two, particularly the second one, seem like they are pretty important to fix before merge.

  • the missing left-hand padding is the same for both.

This could be resolved separately.

This seems to me to be three separate issues that need to be opened for KeymanWeb.

jahorton commented 1 month ago
  • 16.0 does not affect page text, whereas 17.0 does

These two, particularly the second one, seem like they are pretty important to fix before merge.

Digging into this point: the naijatype keyboard has the following in its associated CSS:

body {
    background: rgb(193, 194, 193);
    font-family: "Andika","DejaVu Sans","AndikaAfr","Andika Afr","Andika New Basic Compact";
}

https://github.com/keymanapp/keyboards/blob/03b932e9b9558b6885534d31934084d00e88a6ef/experimental/n/naijatype/source/naijatype.css#L6-L9

The "affecting page text" issue is a consequence of this - we wish to preserve OSK styling, and there is some actual OSK-styling content in that CSS file. It's just that some of it is decidedly not targeted for the OSK.

jahorton commented 1 month ago

Things look fine for a number of other keyboards; I think the observed issues are due to interactions with that keyboard's custom CSS. If I disable use of its CSS on that page (by using a build that doesn't add it), the page is fine outside of a couple of spacebar text-size issues.

As for spacebar text, the layout-refresh isn't properly getting the correct final width for the doc-keyboard; it's defaulting to 800 instead of 960, 520, or 720 corresponding to the emulated form-factor. So, for phone layouts, it thinks the spacebar's width proportion is out of 800 pixels, not 520 pixels.

mcdurdin commented 1 month ago

Digging into this point: the naijatype keyboard has the following in its associated CSS:

I've opened keymanapp/keyboards#3039 to address this.