MaximBelov commented on PR #1073: URL: https://github.com/apache/cordova-plugin-inappbrowser/pull/1073#issuecomment-5282414506
Thanks for taking it forward — but I'd like to push back on swapping the horizontal progress bar for a centred indeterminate spinner. `onProgressChanged` hands us a real 0–100 value. The bar used it; the spinner reduces it to a boolean, from the same callback, for the same lines of code. On Android a thin determinate bar under the toolbar is also the platform idiom — Chrome, Custom Tabs and Samsung Internet all do it — and unlike a centred overlay it doesn't sit on top of content the user is already reading on every subsequent in-page navigation. The black background you noticed is largely a symptom of the indicator being centred in an otherwise empty view; the actual fix for that is #1167. I'd argue "inline with iOS" belongs in the option name rather than in the pixels. `closebuttoncaption` is already an X icon on Android and a Done text button on iOS under one key, and `footer` is documented as "similar to the iOS Done button" — the README keeps separate per-platform option lists precisely because rendering is native. It's also worth noting that iOS has had the spinner since the initial import of the native iOS code (cc149748, May 2013), which targeted UIWebView — a control with no page-progress API at all. WKWebView and `estimatedProgress` only arrived with iOS 8. So the spinner was never chosen over a bar; a bar wasn't available. So: happy to keep your `hidespinner` key — one option, documented once per platform, no new API — and render it on Android as the horizontal determinate bar. I'll also hide it in `onPageFinished`, `onReceivedError`, `onDownloadStart` and `onReceivedSslError`: right now the only hide condition is `progress == 100`, so a download-triggering link or a cancelled SSL handshake leaves it running indefinitely, which is the error case #1076 explicitly asks about. Also worth flagging: the PR description still describes the original implementation — it says nothing is added to the layout when the option is set, which was true when `main.addView` was guarded, but the spinner is now attached unconditionally and left `GONE`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
