Fix back button when app is inside Cozy #3909
Conversation
| @@ -345,6 +349,7 @@ class MailboxDashBoardController extends ReloadableController | |||
| } | |||
| _registerStreamListener(); | |||
| BackButtonInterceptor.add(onBackButtonInterceptor, name: AppRoutes.dashboard); | |||
| registerCozyPopState(); | |||
| if (PlatformInfo.isMobile) { | ||
| if (currentContext != null && canBack(currentContext!)) { | ||
| return false; | ||
| } else { | ||
| clearSelectedEmail(); | ||
| return true; | ||
| } | ||
| } else { | ||
| clearSelectedEmail(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
combine conditions to avoid duplicated
| super.onClose(); | ||
| } | ||
|
|
||
| Future<void> registerCozyPopState() async { | ||
| final isInsideCozy = await CozyConfigManager().isInsideCozy; |
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/3909. |
| } | ||
|
|
||
| _popStateDebouncer = Debouncer( | ||
| const Duration(milliseconds: 100), |
There was a problem hiding this comment.
It smells like a workaround and later a race condition.
When we inject that to chat https://github.com/linagora/linagora-design-flutter/blob/master/lib/cozy_config_manager/cozy_config_manager_web.dart#L57 it works. And it should only be like that.
Here you "just" debounce to make it works, right? I don't think this is the correct way to fix that.
Maybe you should work closely with @zatteo to fix this issue 🙏
Idea: maybe this is because you don't have navigation history on mail (you seems to replace URL instead of pushing), so the iframe's parent take the event first?
There was a problem hiding this comment.
Also, it seems that with Chrome, on the web version, without Cozy, back button works after reading an email. But it doesn't work with Firefox.
So maybe there is really something buggy there
There was a problem hiding this comment.
You are right, this is a work around. I've spent almost 2 days on this without further progress.
There was a problem hiding this comment.
Sometimes workaround has to be done and I have no pb with that. But when we add a workaround commit message should reflect it, code too.
Why do we replaceBrowserHistory at first? I saw this commit f4ffd11 but nothing that explain why we move from push to replace.
Because of replace, we need to intercept back button rather than leaving it to the browser. Sometimes we really need to replace, but maybe not in all case.
If with a debounce there is something working (does it works with chrome & firefox as well?), then it means that we call several times a method that we don't have too.
Also, since we can reproduce the issue with firefox in a standalone mode (without cozy), maybe it'll be easier to debug?
There was a problem hiding this comment.
With standalone app, it seems that Firefox and Chrome have different heuristics when using back button. Firefox is skipping more history items that Chrome.
| standalone app url | Firefox | Chrome |
|---|---|---|
| 1 - https://google.com (my previous URL for testing purpose) | 1st back button goes here directly | 2nd back button goes here |
| 2 - https://mail.linagora.com/ | ||
| 3 - https://mail.linagora.com/dashboard?type=normal | ||
| 4 - https://mail.linagora.com/dashboard?type=normal&context=fc3c7dc0-e7be-11ef-8231-254b1f223ea1 | 1st back goes here | |
| 5 - https://mail.linagora.com/dashboard/8975a390-6c5a-11f0-9363-71acbfa1bbeb?type=normal&context=fc3c7dc0-e7be-11ef-8231-254b1f223ea1 |
This different heuristics may be an issue, but it does not explain why it is different (and a total mess) in Cozy iframe.
| with cozy iframe url | Firefox | Chrome |
|---|---|---|
| 1 - https://google.com/ (my previous URL for testing purpose) | 2 | 5 |
| 2 - https://tpoizat-mail.twake.linagora.com/ | ||
| 3 - https://tpoizat-mail.twake.linagora.com/#/bridge/ (this is obviously linked to bridge) | 2 3 | |
| 4 - https://tpoizat-mail.twake.linagora.com/#/bridge/dashboard?type=normal | 1 | 1 |
| 5- https://tpoizat-mail.twake.linagora.com/#/bridge/dashboard?type=normal&context=fc3c7dc0-e7be-11ef-8231-254b1f223ea1 | 4 | |
| 6 - https://tpoizat-mail.twake.linagora.com/#/bridge/dashboard/8975a390-6c5a-11f0-9363-71acbfa1bbeb?type=normal&context=fc3c7dc0-e7be-11ef-8231-254b1f223ea1 |
I am trying to understand more why it behaves differently. I think the Uncaught TypeError: e.data.includes is not a function is already a good part of the fix no?
There was a problem hiding this comment.
After more investigation, it seems that this code is responsible of the difference of behavior between in iframe/not in iframe https://github.com/cozy/cozy-libs/blob/04447f2c409922ea10f0859c31536a441ca009ac/packages/cozy-external-bridge/src/embedded/index.ts#L36
I'm still looking for :
- why
replaceStateis an issue and notpushState - why there is no issue on Twake Chat
- if I have an alternative way of listening to history updates (but for the moment I think not)
There was a problem hiding this comment.
I will try to replace replaceState of tmail to pushState to test the behavior.
There was a problem hiding this comment.
@Crash-- discovered the source of the issue. I replaced by mistake replaceState by pushState in the embedded part of the bridge. See the fix here cozy/cozy-libs#2804.
So if you update cozy-external-bridge to 0.16.1, back button in iframe should work like back button standalone. I tested it locally with success. I am really sorry for wasting your time with this bug.
There was a problem hiding this comment.
Confirmed worked. Demo updated. PR updated.
tddang-linagora
force-pushed
the
notion/cozy-browser-back-fix
branch
from
337a3ff to
e5dbc6e
Compare
tddang-linagora
temporarily deployed
to
PR-3909
GitHub Actions
Inactive
|
tddang-linagora
force-pushed
the
notion/cozy-browser-back-fix
branch
from
1d22a8a to
57a8674
Compare
Issue
Dependency
Demo
cozy-back.mov