[$250] Web - Notification count in the tab flashes when back is clicked

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Issue found when executing PR #29409

Version Number: v1.3.84-0

Reproducible in staging?: Y

Reproducible in production?: N

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Expensify/Expensify Issue URL:

Issue reported by: Applause-Internal Team

Slack conversation: @

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Sign in to an account.
3. Go through different unread chats.
4. Click back.
5. Observe the Notification count in the tab.

Expected Result:

Notification count should be the same

Actual Result:

Notification count in the tab flashes when back is clicked

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• Windows: Chrome
• MacOS: Desktop

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Bug6236815_1697275265599.notification_count.mp4

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011757bc5f159e3e7a
• Upwork Job ID: 1713187815326801920
• Last Price Increase: 2023-10-15

[melvin-bot]

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~011757bc5f159e3e7a

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @akinwale (External)

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @Li357 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[izarutskaya]

Nor teproducible in production

bandicam.2023-10-14.17-00-23-295.mp4

[pradeepmdk]

Nor teproducible in production

bandicam.2023-10-14.17-00-23-295.mp4

I think this is not a bug because when you hit the back button notification count is wrong in Production

this flicker will happen when we update the new count.

so here we are fixing when you hit the back button we are updating the correct count #29409
cc: @sobitneupane @MariaHCD

[ikevin127]

I can still reproduce this on latest main on MacOS: Chrome.

The reason why some might not be able to reproduce this is because it's a Chromium engine specific issue hence why @pradeepmdk might have missed it in his PR #29409

Video

MacOS: Chrome

Screen.Recording.2023-10-14.at.19.08.32.mov

Proposal

Please re-state the problem that we are trying to solve in this issue

Notification count in the tab flashes when back is clicked.

What is the root cause of that problem?

This is due to the way Chromium based browsers handle their tab management - it maintains a snapshot of the title and favicon and re-uses it briefly when navigating.

Browsers do have some freedom in exactly when they fire popstate events, and Chrome seems to do it before the new page has fully loaded - hence the flickering of notification count on Chrome.

The issue is caused by the logic in this file:

App/src/libs/UnreadIndicatorUpdater/updateUnread/index.website.js

Lines 6 to 14 in fe282b4

	let unreadTotalCount = 0;
	/**
	* Set the page title on web
	*
	* @param {Number} totalCount
	*/
	function updateUnread(totalCount) {
	const hasUnread = totalCount !== 0;
	unreadTotalCount = totalCount;

Because this does not handle Chrome's behaviour when it comes to tab management and therefore on Chrome the state of the snapshot is keept hence the flash of old notification count before being updated with the current one.

What changes do you think we should make in order to solve the problem?

My approach which will handle this issue for all browsers (including Chromium based) is to use window's history.replaceState() to replace the current history instead of a new one being added, therefore when clicking the back button it won't show the old snapshot count title and instead will show the correct notification count without any flashes.

- let unreadTotalCount = 0;

function updateUnread(totalCount) {
    const hasUnread = totalCount !== 0;
-   unreadTotalCount = totalCount;
    // This setTimeout is required because due to how react rendering messes with the DOM, the document title can't be modified synchronously, and we must wait until all JS is done
    // running before setting the title.
    setTimeout(() => {
        // There is a Chrome browser bug that causes the title to revert back to the previous when we are navigating back. Setting the title to an empty string
        // seems to improve this issue.
        document.title = '';
        document.title = hasUnread ? `(${totalCount}) ${CONFIG.SITE_TITLE}` : CONFIG.SITE_TITLE;
        document.getElementById('favicon').href = hasUnread ? CONFIG.FAVICON.UNREAD : CONFIG.FAVICON.DEFAULT;

        // Update the browser history with a new title and favicon, instead of letting it remember the old state
        // This is used because a side effect in Chromium based browsers where browser creates snapshots
        // of the page state and re-uses them when navigating, causing a brief "flash" of the old title/favicon
+       window.history.replaceState({unreadTotalCount: totalCount}, document.title);
    }, 0);
}

// Update the title and favicon when the user navigates to a history state that we pushed.
- window.addEventListener('popstate', () => {
+ window.addEventListener('popstate', event => {
+   if (!((event.state && "unreadTotalCount" in event.state))) {
+       return;
+   }

-    updateUnread(unreadTotalCount);
+   updateUnread(event.state.unreadTotalCount);
});

What alternative solutions did you explore? (Optional)

N/A

Videos

MacOS: Chrome

Screen.Recording.2023-10-14.at.18.37.15.mov

Screen.Recording.2023-10-14.at.18.35.46.mov

[pradeepmdk]

@ikevin127 see the notification count in the resulting video when you hit back count increases.

[akinwale]

@ikevin127 Your proposed solution has a flaw where the unread count in the browser title ends up being incorrect. It should maintain the current unread count after clicking the back button.

In any case, if valid, this may end up being considered as a regression of the PR #29409 which would require the original PR author to fix it.

We'll have to wait for feedback from an internal engineer.

cc @MariaHCD @sobitneupane

[ikevin127]

Indeed, I know that the counter increases when going back from reading a notification.

Seeing this comment's video #29604 (comment) I thought that's the intended behaviour as that's how it is currently on staging.

[melvin-bot]

Upwork job price has been updated to $250

[mountiny]

I agree this is not a blocker so removing the label, however we should still investigate this unless it will be covered by @pradeepmdk PR

[MariaHCD]

I do believe this is a regression from #29409 so it makes sense for the PR author @pradeepmdk to handle.

cc: @sobitneupane

[pradeepmdk]

@MariaHCD I think this is not a regression this is expected behavior only.
example Gmail app notification count https://www.loom.com/share/3ddc714923c94b04a689d3eecdd081ae?sid=7484276b-0984-4a1a-a27d-d7f662f55e9a (here also we can see that flashes )

Here when we click the browser back button it will load from browser history at the time in the navigation state whatever stored it retrieved. so the count is not updated in history so we are updating the new count while clicking the browser button. when we update the new count more fast flashes occur.

we can use setTimeout for delay updates but that is not a good practice.

[ikevin127]

I can confirm that this is not a regression. I consider this a browser level limitation / issue.

Browsers that use the Chromium engine keep snapshots of pages for performance reasons and to improve the user experience when navigating through the history. These snapshots include the current state of the DOM, the JavaScript heap, and other details such as the window title.

When you navigate back to a page using the forward/backward buttons, or via JavaScript's history API, these snapshots are used to quickly reconstruct the page to the state it was in when the snapshot was created.

This means that any changes you make to the window title or other browser-level features in the popstate event (or other events that occur after the snapshot was taken but before the page is visible to the user again) can be overwritten by the old, cached state from the snapshot while the page is loaded. This could result in a brief "flash" of the old title before your new title is set.

Unfortunately, this behavior is a side effect of how browsers implement page navigation for performance, and is unlikely something that can be easily changed or worked around. It could be seen as a browser level limitation or issue, but not necessarily a "bug".

[sakluger]

There is some disagreement around whether this is a regression. @MariaHCD what do you think about what @pradeepmdk and @ikevin127 are saying?

[MariaHCD]

Oh, interesting. Thanks @pradeepmdk and @ikevin127! This does look like a browser-level limitation. So not a regression, then.

Perhaps there won't be a clean solution for this so I'd say if we don't find a good solution, we can close this one out.

Thoughts, @akinwale?

[akinwale]

@MariaHCD Since it's not a regression, the proposal by @ikevin127 could work in this scenario using history.replaceState (reference), where the unread count is being stored in the browser's history state.

@ikevin127 Could you please update your proposal and provide a video to demonstrate the expected behaviour with the unread count? Is it also possible to get rid of the setTimeout or is that still required for it to work properly?

[ikevin127]

@akinwale There's a catch with my current proposal:

• when you hit the back button the notification count increments back up even though the notifications were read
  which from what I was told it is not the desired behaviour

MacOS: Chrome Example

Screen.Recording.2023-10-14.at.18.35.46.mov

So there's a choice here to be made between the following options:

1. We keep @pradeepmdk's current implementation which causes the flash on Chrome browser
2. We add my proposal's solution for Chrome browser only, all other browsers keep above (1.) solution except Chrome
3. We add my proposal's solution for all browsers (least desirable)

Based on what's decided here I can update my proposal.

As for getting rid of the current implementation's setTimeout I'm not sure, according to the developer's comment:

// This setTimeout is required because due to how react rendering messes with the DOM, the document title can't be modified synchronously, and we must wait until all JS is done running before setting the title.

we might break the current functionality if removed. I'll make sure to check when updating my proposal if we get there.

[akinwale]

2. We add my proposal's solution for Chrome browser only, all other browsers keep above (1.) solution except Chrome

If we go with this (we can use Browser.isChrome to apply Chrome-only behaviour), will the correct unread count (ie. the reduced count) be displayed as expected without flashing?

[ikevin127]

To answer your question: no.

After further testing on the current implementation that's on staging I can confirm that on Chrome the following happens:

• on back / forward button the notification flickers to cached title before the real count is being shown (current issue)
• also for the following steps:

1. Go to https://staging.new.expensify.com
2. Sign in to an account
3. Go through different unread chats in order for the tab notification to decrement count (1-2 decrements)
4. Logout
5. Click browser forward button and observe the same flicker will happen briefly showing cached title before showing just logged out title: New Expensify without any notification count

So I guess that this flicker thing was always there on Chrome as the PR author showed here for Gmail and this PR #29409 only exposed it to where it can be observed.

I wouldn't count this behaviour as a regression, as I said before its more of a browser-level limitation.

Solution if the flicker is really not wanted while logged in and navigating back / forward:

The only way to get rid of the flicker would be to allow the notifications to increment back up on Chrome while logged in. If the user reads some messages then performs logout and then hits the forward button, we will keep the current implementation with the flicker because when upon logout it's less likely that users would hit the forward button and see the flicker. It's more likely to be seen while logged in and hitting back button while reading through reports.

^ - This would be the behaviour for Chromium based browsers. For all other browsers the current implementation works as expected, no flickers both logged in back / forward buttons and upon logout back / forward buttons.

Would look like this on Chrome

Screen.Recording.2023-10-18.at.00.14.32.mov

If this solution is something that would work for you, I can update my proposal with the solution described above.
Otherwise, I think keeping the current implementation works just fine.

[akinwale]

@MariaHCD Considering the discussion above, looks like we may have to close this since it's a browser limitation.

[MariaHCD]

I'd agree with closing since it's a browser limitation. Leaving the final decision to the assigned folks here, @Li357 and @MitchExpensify

[MitchExpensify]

lets do it