fix: rewrite /keyboards for embed mode and skip localization#734
Conversation
Keyman 14.0 - 18.0 have a dependency on /keyboards which means that we cannot rewrite URLs there when we are in 'embed' mode for those apps. The apps check if the URL starts with /keyboards, and handle those paths internally, while opening other URLs in an external browser. See keymanapp/keyman#15948 for platform-specific dependencies on this behaviour and for v19.0 planned mitigations. Also remove _ie_thunk and _legacy from .htaccess as these are no longer required following removal of pre-Keyman-13.0 support. Relates-to: keymanapp/keyman#15948
User Test ResultsTest specification and instructions User tests are not required |
| $('.title a', k).text(kbd.name).attr('href', `/${I18n.pageLocale()}/keyboards/h/${kbd.id}${embed_query_q}`); | ||
| $('.title a', k).text(kbd.name).attr('href', `${page_root}/h${kbd.id}${embed_query_q}`); |
There was a problem hiding this comment.
Note deletion of extra / after h which was previously incorrect, because the dedicated landing pages always have a preceding /:
keyman.com/cdn/dev/keyboard-search/dedicated-landing-pages.js
Lines 5 to 7 in b95a4a6
| RewriteCond "%{HTTP_COOKIE}" embed_keyboards_no_locale_redirect [OR] | ||
| RewriteCond "%{QUERY_STRING}" embed | ||
| RewriteRule "^keyboards/install/([^/]+)$" "/_content/keyboards/install.php?id=$1" [END,QSA] |
There was a problem hiding this comment.
The pattern here is similar through this section:
- Add the two
RewriteCondlines ([OR]flag reduces number of rules!) - Strip the BCP47 component from the path
- Adjust
$2references to$1.
darcywong00
left a comment
darcywong00
left a comment
There was a problem hiding this comment.
lgtm.
For 19.0 apps, would we tweak /go/ links in #730?
| $session_query_q = "?$session_query"; | ||
| } else { | ||
| $session_query = ''; | ||
| $session_query_q = ''; |
There was a problem hiding this comment.
Do we need to delete the cookie?
There was a problem hiding this comment.
No, it's a session cookie, so will expire by itself when the session ends.
We'll need to design the scheme carefully and document that in keymanapp/keyman#15948. I think we can address that all separately, after epic/web-i18n lands. The apps will need refreshing to support the new pathways. |
3 participants
Keyman 14.0 - 18.0 have a dependency on /keyboards which means that we cannot rewrite URLs there when we are in 'embed' mode for those apps. The apps check if the URL starts with /keyboards, and handle those paths internally, while opening other URLs in an external browser.
See keymanapp/keyman#15948 for platform-specific dependencies on this behaviour and for v19.0 planned mitigations.
Also remove _ie_thunk and _legacy from .htaccess as these are no longer required following removal of pre-Keyman-13.0 support.
Relates-to: keymanapp/keyman#15948
Test-bot: skip