fenix icon indicating copy to clipboard operation
fenix copied to clipboard

Close #26150: Do not show sync tabs in home when sync open tabs setting is disabled

Open rocketsroger opened this issue 3 years ago • 3 comments

Pull Request checklist

  • [x] Tests: This PR includes thorough tests or an explanation of why it does not
  • [ ] Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • [ ] Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

  • [x] QA Needed

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-debug task.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Fixes #26150

rocketsroger avatar Aug 11 '22 17:08 rocketsroger

@sarah541 FYI, I think this touches some of the files you are also currently working on

MatthewTighe avatar Aug 11 '22 17:08 MatthewTighe

Seeing some issues while testing. This PR will need an update before landing.

rocketsroger avatar Aug 11 '22 18:08 rocketsroger

LGTM, but @MatthewTighe might want to take a quick look at it before it lands as well since he has the most up-to-date context. 🙂

jonalmeida avatar Aug 11 '22 22:08 jonalmeida

Putting this on hold. From some discussions we might have a better fix that will fix all the place holder problems.

rocketsroger avatar Aug 12 '22 19:08 rocketsroger

This latest push looks like it reintroduces the original issue. If I have a synced tab available and disable tabs syncing in settings then the tab will still be visible when I return to home. It looks like the code to check the engines storage was removed, so the AppStore will never be updated with new state.

Yes, it stays until the next time the user starts the app. I was looking for a simple change that remove the problem with blank sync tab card but not more. Do you think we have to hide the sync tab if we already have it in memory?

rocketsroger avatar Aug 15 '22 22:08 rocketsroger

Yes, it stays until the next time the user starts the app. I was looking for a simple change that remove the problem with blank sync tab card but not more. Do you think we have to hide the sync tab if we already have it in memory?

I think in general if a user disables tab syncing it is probably with the intent that synced tabs are no longer available on their device, so they might be surprised to see it still on their home screen. That case can be handled in a follow-up though

MatthewTighe avatar Aug 15 '22 23:08 MatthewTighe

I think in general if a user disables tab syncing it is probably with the intent that synced tabs are no longer available on their device, so they might be surprised to see it still on their home screen. That case can be handled in a follow-up though

Sure, I'll restore the part where we check if the tab sync is off. Thanks

rocketsroger avatar Aug 15 '22 23:08 rocketsroger