Skip to content

Conversation

brianblanchard-wf
Copy link
Contributor

@brianblanchard-wf brianblanchard-wf commented Nov 1, 2016

Description

  • When an app using this package is run inside a WKWebView and checks the version an error will be throw.
  • The error is thrown because there is no Version/X.X.X string in the string returned from navigator.appVersion when running within a WKWebView.

What Was Changed

  • Added a stricter check for isSafari
  • Added a Browser subclass for the WKWebView

@travissanderson-wf @dustyholmes-wf @kerbyseeley-wf

@aviary2-wf
Copy link

Raven

Number of Findings: 0

@rmconsole-wf
Copy link

rmconsole-wf commented Nov 1, 2016

	When this pull is merged I will use the following information:
	Version: platform_detect 1.1.0
	Release Ticket(s): CP-2835

Last updated on Tuesday, January 03 09:43 AM CST

@codecov-wf
Copy link

codecov-wf commented Nov 1, 2016

Current coverage is 89.87% (diff: 100%)

Merging #8 into master will increase coverage by 1.99%

@@             master         #8   diff @@
==========================================
  Files             2          2          
  Lines            66         79    +13   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits             58         71    +13   
  Misses            8          8          
  Partials          0          0          

Sunburst

Diff Coverage File Path
•••••••••• 100% lib/src/browser.dart

Powered by Codecov. Last update 6ab96fe...5220a0f

@travissanderson-wf
Copy link
Contributor

FYI @clairesarsam-wf

_safari,
_internetExplorer
_internetExplorer,
_wkWebView
Copy link
Contributor

Choose a reason for hiding this comment

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

@clairesarsam-wf @robbecker-wf what do you guys think about detecting these "sub" type browsers like WkWebView is for the most part the same as Safari, similar to Rob's question about separating the detection of IE vs Edge from earlier. When WKWebView is detected, should isSafari return true as well as isWKWebView?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@travissanderson-wf We could do that but the issue is if we return true for isSafari in the WKWebView the there would not be a version because the Safari and the WKWebView have different version strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

it could still have a version, currently it would be different depending on if it was parsed out by _Safari or _WKWebView, I just mean we could change the getter for isSafari = this == _safari || this == _wkWebView

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I could see it being useful to know if you're running in a webview.


script:
- pub get
- pub run dart_dev format --check
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greglittlefield-wf says UIP does not use the dart formatter so it has been removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

what does that have to do with this repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @travissanderson-wf, I was under the impression that UIP was now maintaining this repo.

Brian was running into issues with builds failing due to the formatter enforcing an 80 line length, so I told him to just remove the formatter step from the build as opposed to fixing the issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, it's presently officially owned by CP but so far it's just been me and I don't expect a whole lot of churn here TBH. In the interest of consistency, I'd prefer to keep dartfmt enforced. It's pretty easy to fix the build with ddev format :)

@travissanderson-wf
Copy link
Contributor

I'm good with this as-is but would be curious to hear others' input on whether WkWebView should report as both Safari and WKWebView or only WKWebView. My suspicion is that code checking for Safari should probably also run when you're in a WKWebView and so it would be desirable as the consumer if isSafari checks would be true in that case. Special cases for WKWebView could still explicitly check for isWKWebView.

@robbecker-wf
Copy link
Member

robbecker-wf commented Nov 2, 2016

I'm of the opinion that isSafari should return true for wkwebview and a webview property it is a more specific version of the main browser type. For instance, it may be possible that:
isChrome => true && isWebView => true for when running in a chrome webview.

Not to bikeshed, but perhaps we should just renamed wkWebView to just webView ?

UPDATE: To be clear, I'm ok with this PR as-is, just my 2c.

@travissanderson-wf
Copy link
Contributor

travissanderson-wf commented Nov 2, 2016 via email

@brianblanchard-wf
Copy link
Contributor Author

@robbecker-wf @travissanderson-wf I think that it would be good to detect webviews for sure but in a different ticket.

@travissanderson-wf
Copy link
Contributor

+1

@travissanderson-wf
Copy link
Contributor

@brianblanchard-wf can you add a QA p1 (assuming this fixes your issue)?

@travissanderson-wf
Copy link
Contributor

@kerbyseeley-wf is also taking a look at it from a datatables perspective

@kerbyseeley-wf
Copy link

+10 as far as datatables is concerned. Ran a smoke test in Chrome/Firefox/Safari/IE.

@jordanross-wf
Copy link

jordanross-wf commented Nov 7, 2016

QA +1

@travissanderson-wf
Copy link
Contributor

rosie

@travissanderson-wf travissanderson-wf merged commit 889f27d into Workiva:master Nov 7, 2016
@teresarevious-wf
Copy link

RM +1 Manually verified dependencies and configured correctly to scan for them

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.

10 participants