firefox-ios icon indicating copy to clipboard operation
firefox-ios copied to clipboard

Add FXIOS-6047 - iOS No Longer Asking For Permission To Paste URL

Open tisumi99 opened this issue 1 year ago • 1 comments

:scroll: Tickets

Jira ticket Github issue

:bulb: Description

Replaced UIButton with UIPasteControl in URL toast for iOS 16 and above. This fixes the attached issue as the app will no longer read the clipboard without user interaction. Clicking on UIPasteControl gives the app permission to read the clipboard. Issue does not exist prior to iOS 16 as UIPasteControl and the clipboard security measures were released in iOS 16.

  • Added subclass PasteControlToast: ButtonToast. Subclass replaces UIButton with UIPasteControl.

  • Added method BrowserController.paste(). This method is called after UIPasteControl is cliked. Method gets the URL from the clipboard and opens a new browser tab with the URL.

  • Added method BrowserViewController.shouldDisplay() as delegate for ClipboardBarDisplayHandler. Method sets view.pasteConfiguration and configures URL toast.

  • Added branched code to ClipboardBarDisplayHandler.checkIfShouldDisplayBar() to call BrowserViewController.shouldDisplay() instead of BrowserViewController.shouldDisplay(clipBoardURL:) if iOS 16 or greater. The branched code does not read the clipboard as that would trigger iOS to ask for user permission. Instead, app waits until UIPasteControl is clicked to read the clipboard.

  • Added method ClipboardBarDisplayHandler.shouldDisplayBar(pasteBoardChangeCount:). Since app cannot read the clipboard without iOS asking for user permission, app is now using UIPasteboard.general.changeCount to check if the clipboard contents have changed. Checking changeCount does not required user permission.

:pencil: Checklist

You have to check all boxes before merging

  • [x] Filled in the above information (tickets numbers and description of your work)
  • [x] Updated the PR name to follow our PR naming guidelines
  • [x] Wrote unit tests and/or ensured the tests suite is passing
  • [ ] When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • [ ] If needed, I updated documentation / comments for complex code and public methods
  • [ ] If needed, added a backport comment (example @Mergifyio backport release/v120)

tisumi99 avatar May 10 '24 18:05 tisumi99

Screenshot 2024-05-10 at 11 23 56 AM

Before - iOS Asks User Permission Repeatedly

https://github.com/mozilla-mobile/firefox-ios/assets/98071325/9fa502ab-4894-4868-8ab3-02836f8402d9

After - iOS No Longer Asking Permission

https://github.com/mozilla-mobile/firefox-ios/assets/98071325/521762ec-7be3-414b-91fa-0b437a6f12d5

tisumi99 avatar May 10 '24 18:05 tisumi99

Thanks for this PR @tisumi99! Can you clarify the before/after UI, it looks like a few changes have been made (for ex. we're no longer displaying the destination URL to the user in the toast descriptionText, and the button has changed etc.). These changes seem fine but they're not listed in the ticket and so I was hoping to just confirm where these decisions on the UI originated from?

mattreaganmozilla avatar May 13 '24 23:05 mattreaganmozilla

@mattreaganmozilla We can no longer display the URL on the toast as the app should not read the pasteboard before the user clicks on UIPasteControl.

The UI for UIPasteControl is provided by Apple and is not very customizable. The background color, foreground color, and corner radius are configurable and set to the current theme. However, the "Paste" text and font cannot be changed. We do have the option of not displaying the icon next to "Paste" but I left the icon as that seems to be the default and users may already be familiar with the icon from seeing it in other apps.

This blog post does a pretty job summarizing the pasteboard security changes that happened in iOS16: https://sarunw.com/posts/uipasteboard-privacy-change-ios16/

Links to UIPasteControl documentation for convenience: https://developer.apple.com/documentation/uikit/uipastecontrol https://developer.apple.com/documentation/uikit/uipastecontrol/configuration

tisumi99 avatar May 13 '24 23:05 tisumi99

Thanks @tisumi99, I understand the rationale for the changes, just wanted to clarify if there had been discussions outside of the ticket on the UI changes, since the update here differs from the expected behavior listed in the ticket afaics (though I don't have 100% context on that). This LGTM though, I'm reaching out to the team to confirm if there are any other steps that may be needed for this UI update.

mattreaganmozilla avatar May 14 '24 01:05 mattreaganmozilla

@mattreaganmozilla There were no discussions outside of the ticket.

I originally did have a fix that did as the ticket asked for. I had ClipboardBarDisplayHandler register a nil from the pasteboard to mean that the user did not give permission to read the pasteboard as there was no other way to know if the user denied permission. Once the nil was registered, ClipboardBarDisplayHandler would never try to read the pastboard again until the app was restarted. This felt like a work around to deal with the iOS 16 changes and users would have to restart the app if they decided later that they do want to paste URLs. Also in the case where the user gave permission, the user would have to double confirm that they wanted to paste the url (once in the iOS alert, and again in the toast). So it seemed to make more sense to implement UIPasteControl to 1) address the ticket, 2) address the double confirm issue, and 3) conform to the updated pasteboard security protocol.

The only other option I can think of is to not make any changes to the app and let users know that they have to give Firefox permission to read the pasteboard in iOS settings to avoid constantly being asked for permission. Asking for permission each time the pasteboard is read is the default setting.

tisumi99 avatar May 14 '24 01:05 tisumi99

Thanks again @tisumi99 for your work on this. 👍 The change makes sense to me (and thank you for the well-written PR description and screenshots). I'm just double-checking a couple things, should have it reviewed shortly.

mattreaganmozilla avatar May 14 '24 16:05 mattreaganmozilla

@tisumi99 Would you mind rebasing this branch against current main to see if that fixes the CI failure? Thank you

mattreaganmozilla avatar May 15 '24 20:05 mattreaganmozilla

Hi @tisumi99, it looks like something may have gone awry with the the rebase you made (diff is now showing 289 files changed). Can you please try to correct it? If you run into further difficulties you can alternatively open a new PR if needed (cherry-pick your original changes to it, and then just message me the new PR link here), and we can close this one.

mattreaganmozilla avatar May 15 '24 22:05 mattreaganmozilla

@mattreaganmozilla Oops, first time doing a rebase. I fixed the PR.

tisumi99 avatar May 15 '24 23:05 tisumi99

Warnings
:warning: Variable 'crossCircleFill' in Medium is out of alphabetical order.
:warning: Variable 'cross' in Medium is out of alphabetical order.
Messages
:book: Project coverage: 32.31%
:book: Edited 3 files
:book: Created 0 files

Client.app: Coverage: 30.92

File Coverage
BrowserViewController.swift 4.43% ⚠️
ButtonToast.swift 0.0% ⚠️
ClipboardBarDisplayHandler.swift 0.0% ⚠️

Generated by :no_entry_sign: Danger Swift against 30a0d136118accf977173c6b02283d434e9632d6

mobiletest-ci-bot avatar May 15 '24 23:05 mobiletest-ci-bot

@tisumi99 Just wanted to let you know I haven't forgotten about this PR, I'm waiting to get confirmation on the UI changes with our UX team which is something we try to do whenever UI is updated. As soon as I hear back I'll update. Thanks again for this PR.

mattreaganmozilla avatar May 17 '24 15:05 mattreaganmozilla

Thanks for the update @mattreaganmozilla .

tisumi99 avatar May 17 '24 16:05 tisumi99

@tisumi99 Still waiting for confirmation from UX team (they like to review UI changes whenever possible before merging). That process can sometimes take a while, but I'll update as soon as I hear back. Thank you

mattreaganmozilla avatar May 22 '24 18:05 mattreaganmozilla

This PR has been automatically marked as stale. Please leave any comment to keep this PR opened. It will be closed automatically if no further update occurs in the next 7 days. Thank you for your contributions!

github-actions[bot] avatar Jun 06 '24 00:06 github-actions[bot]

@tisumi99 Apologies for the ongoing delays, I'm still working on getting confirmation from our UX team. PR is still on my radar.

mattreaganmozilla avatar Jun 06 '24 16:06 mattreaganmozilla

Thanks again @tisumi99 for this PR, merging now.

mattreaganmozilla avatar Jun 20 '24 19:06 mattreaganmozilla