Safari specific voice search alert message #1513
Conversation
This commit makes the "Note: for Safari users, Siri must be enabled" message only appear on Safari, using user-agent sniffing. Generally speaking you want to avoid UA sniffing where you can, but here it doesn't seem like there are any alternatives. Instead of writing unit tests for specific UA strings, which aren't particularly useful for making sure your sniffing works in a real scenario (since issues normally arise from the strings becoming out of date, NOT somebody changing the source code), I added a useragent acceptance test suite. This suite spins up instances of specific browsers, and checks that the sniffer methods work as expected on each of them. If need be, we can run these tests on specific browser versions. J=SLAP-1522 TEST=manual,auto test that the Safari specific not only shows up in Safari I couldn't figure out how to naturally get the 'service-not-allowed' error to show up on safari/chrome, so I changed the error switching code to always go into this case, and then disconneted from wifi to trigger the 'service' error.
oshi97
force-pushed
the
dev/safari-specific-voice-search-msg
branch
from
394f880
to
503bee7
Compare
| const [userAgent, userAgentData] = await ClientFunction(() => { | ||
| return [window.navigator.userAgent, window.navigator.userAgentData]; | ||
| })(); | ||
| global.navigator = { userAgent, userAgentData }; |
There was a problem hiding this comment.
can we not directly access window.navigator.[userAgent|userAgentData] in isSafari or isChrome? is navigator different from window.navigator
There was a problem hiding this comment.
We can't run isSafari directly in the browser through testcafe (I tried that initially), ClientFunction doesn't have any reference to the code outside of the browser
There was a problem hiding this comment.
like this is what happens
function someFunctionIWantInTheBrowser() {
console.log('blah');
}
await ClientFunction(() => {
someFunctionIWantInTheBrowser()
// Error is thrown "someFunctionIWantInTheBrowser is not defined"
})();
|
I believe product was okay with simply not including the Safari-specific messaging, so we could go that route if we don't want to add this complexity to the repo. Although now that we have the code for it I think we should move forward with it because I think it will lead to a better UX |
|
Product was able to get the safari message to pop up on iphone on google chrome which was why the QA item was created |
This commit makes the "Note: for Safari users, Siri must be
enabled" message only appear on Safari, using user-agent sniffing.
Generally speaking you want to avoid UA sniffing where you can, but
here it doesn't seem like there are any alternatives.
I tried using https://www.npmjs.com/package/bowser for sniffing,
but that adds an extra 8KB to our modern bundle (128KB -> 134KB).
Our use case is pretty small so it seems a little overkill to use a fully featured
library. Bowser also does not try to use userAgentData internally (AKA user agent clients hints),
which are more robust than the regular user-agent string.
J=SLAP-1522
TEST=manual,auto
test that the Safari specific not only shows up in Safari
I couldn't figure out how to naturally get the 'service-not-allowed'
error to show up on safari/chrome, so I changed the error switching
code to always go into this case.
For Chrome I disconneted from wifi to trigger the 'network' error.
For Edge I ended the voice recognition before it was done to trigger the 'no-speech' error.
Instead of writing unit tests for specific UA strings, which aren't
particularly useful for making sure your sniffing works in a real
scenario (since issues normally arise from the strings becoming out
of date, NOT somebody changing the source code), I added a useragent
acceptance test suite. This suite spins up instances of specific browsers,
and checks that the sniffer methods work as expected on each of them.
If need be, we can run these tests on specific browser versions.
I moved the quarantine-mode flag to only the tests run by CI.
Locally, we typically don't want to run the tests in quarnatine mode.
Also these tests don't need to run in quarantine mode.
The useragentsuite is in a different folder than the other acceptance suites,
because it's meant to be run separately with a specific suite of browsers.