Skip to content

Browser support: Check iOS Safari engine for all iOS browsers#10002

Merged
aduth merged 3 commits intomainfrom
aduth-browser-support-ios
Jan 31, 2024
Merged

Browser support: Check iOS Safari engine for all iOS browsers#10002
aduth merged 3 commits intomainfrom
aduth-browser-support-ios

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jan 30, 2024

🛠 Summary of changes

Adjusts browser support detection to consider all iOS browsers as checking their conformance against the iOS platform version, rather than for the specific browser. Since all browsers on iOS run on Safari (at least for the time being), this should produce a more accurate result for browser support.

This refactors the internal logic to force specific platforms to check a subset of the browser mappings rather than the full set, since in the case of Android WebView and iOS browsers, we only want to check Android or iOS Safari supports explicitly. This avoids having to add additional checks to exclude these platforms in other matchers.

Related Slack discussion: https://gsa-tts.slack.com/archives/C0NGESUN5/p1706209103652959?thread_ts=1706206479.733399&cid=C0NGESUN5

📜 Testing Plan

Verify specs pass:

spec/services/browser_support_spec.rb

Bonus points: Check that error logging script markup is only added to the page based on if eligible iOS browser satisfies the iOS Safari constraint, e.g. overriding the user agent string.

changelog: Internal, Browser Support, Improve browser detection for iOS browsers
Comment on lines +50 to +53
def matchers(mapping)
@matchers ||= browser_support_config.flat_map do |config_entry|
key, version = config_entry.split(' ', 2)
browser_matcher = BROWSERSLIST_TO_BROWSER_MAP[key]
browser_matcher = mapping[key.to_sym]
Copy link
Contributor

Choose a reason for hiding this comment

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

if the passed in mapping is now variable, that will mess with the memoization here? if we memoize based on the result of a cached iOS or Android one, then that will affect future calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, yeah, good call. At least at a glance I feel like that would be an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we memoize the full set of mappings from the constant still and do matchers.slice()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it's exactly what you had in mind, but I think 7511389 should address this. The main challenge is that the matchers shape as an array made it difficult to "pick" specific browser matchers, so I restructured it as a hash.

end
version_test = low_version && ">= #{low_version}"
matcher = proc { |browser| browser_matcher.call(browser, version_test) }
[[key, matcher]]
Copy link
Contributor

Choose a reason for hiding this comment

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

the double array seems weird to me? are we trying to avoid a second iteration if we did next nil + .compact.to_h?

Copy link
Contributor Author

@aduth aduth Jan 30, 2024

Choose a reason for hiding this comment

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

the double array seems weird to me? are we trying to avoid a second iteration if we did next nil + .compact.to_h?

Subconsciously I might have been trying to avoid that, though it's not the most performance-critical code since the whole thing is memoized anyways. I also don't know if I find it especially more or less clear one way or the other.

Another option is a reduce-y like operation? (Probably the most efficient, too)

diff --git a/app/services/browser_support.rb b/app/services/browser_support.rb
index 65983341e2..6ce3030d58 100644
--- a/app/services/browser_support.rb
+++ b/app/services/browser_support.rb
@@ -50,3 +50,3 @@ class BrowserSupport
     def matchers
-      @matchers ||= browser_support_config.flat_map do |config_entry|
+      @matchers ||= browser_support_config.each_with_object({}) do |config_entry, result|
         key, version = config_entry.split(' ', 2)
@@ -54,3 +54,3 @@ class BrowserSupport
         browser_matcher = BROWSERSLIST_TO_BROWSER_MAP[key]
-        next [] if !browser_matcher
+        next if !browser_matcher
 
@@ -60,4 +60,5 @@ class BrowserSupport
         matcher = proc { |browser| browser_matcher.call(browser, version_test) }
-        [[key, matcher]]
-      end.to_h
+        result[key] = matcher
+      end
     end

@aduth aduth merged commit e761ebe into main Jan 31, 2024
@aduth aduth deleted the aduth-browser-support-ios branch January 31, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants