stop-redirecting-in-notification-bar automatically when possible

[fregante]

As discussed in #2952 and #3036

I think stop-redirecting-in-notification-bar can be done automatically if the user opened the notification in a new tab.

For example:

1. disable automatic redirect for all buttons, always
2. try calling history.back() (unless alt is pressed)

If you opened the notification in a new tab, history.back() won't have any effect.

IMHO this makes sense since in that case, the notifications list is still open in its own tab.

PR welcome if you test that this works as expected in both browsers.

cc @FloEdelmann @karlhorky @tonglil

[fregante]

I think this however doesn't work that well/easily on PRs since they have multiple tabs and the bar is persisted. Example:

1. Open PR notification
2. Visit Files tab
3. Press "Done"

You won't be redirected back to the notification list but only back to the Conversation tab. 😕

[FloEdelmann]

Thanks @fregante!

I was already writing an extensive response in #3036 to @tonglil and @karlhorky, but just threw it away because your suggestion is much better!

[karlhorky]

Sounds interesting. I was also going to bring up the potential bugs with history.back() going back to a different page than the notifications.

What about navigating to the Notifications page only if you have a history? Eg:

if (history.length > 1) {
  location.href = 'https://github.com/notifications';
}

[fregante]

OMG I never knew about history.length, I just assumed it was not public information due to security.

It does get more complicated in this situation, but perhaps it's still possible with some ingenuity.

Perhaps it's as easy as:

// Enable behavior only if it's a new tab
if isPR && history.length > 1
  // Remember Notifications List position in history
  notifications = history.length - 1

  // Return to notifications list when the buttons are clicked
  buttons.onclick = () => history.go(notifications - history.length)

It gets a little more complicated when the pages don't load via ajax, so in that case we'd have to preserve this information somehow, maybe:

if (isPR && history.length > 1) || sessionstorage.rghHistory
  ...


  // Store position before navigation
  onbeforeunload = () => sessionstorage.rghHistory = notifications

  // Delete storage if any, it only needs to exist between pageloads
  delete sessionstorage.rghHistory

[karlhorky]

Hm, ok... I'm probably not understanding all of the complexity involved in this. I thought it would be simpler code than that.

Any reason why it wouldn't work with location.href =?

[karlhorky]

Another option would be to leave all of the existing code as it is, and only add the condition of history.length === 1.

So something like:

function handleClick(event: delegate.Event<MouseEvent, HTMLButtonElement>): void {
    // Disable the redirect to the Notifications inbox if either:
    // 1. The alt key was held down during click
    // 2. The history length is `1` (a new tab)
    const redirectDisabled = event.alt || history.length === 1;
    event.delegateTarget.form!.toggleAttribute('data-redirect-to-inbox-on-submit', !redirectDisabled);
}

[FloEdelmann]

leave all of the existing code as it is, and only add the condition of history.length === 1

I think that solution might work well and without much code changes. We just need to store the history.length in the init function, so that navigating around in that new tab (e.g. to the PR's Files tab) while the notification is still visible doesn't change the behavior.

[fregante]

Absolutely! History.go is unnecessary since we have the length now.

However the sessionStorage part enables this on PRs as well, or else this feature won’t work if the user goes to the Files tab after opening a PR: history.length will be 2

[karlhorky]

Got it, so something like the following?

function handleClick(event: delegate.Event<MouseEvent, HTMLButtonElement>): void {
    // Disable the redirect to the Notifications inbox if either:
    // 1. The alt key was held down during click
    // 2. The notification has been opened in a new tab
    const redirectDisabled = event.alt || sessionStorage.rghIsNewTab;
    event.delegateTarget.form!.toggleAttribute('data-redirect-to-inbox-on-submit', !redirectDisabled);
    delete sessionStorage.rghIsNewTab;
}

async function init(): Promise<void> {
    // If the history length is `1` on init, the
    // notification has been opened in a new tab
    sessionStorage.rghIsNewTab = history.length === 1;
	delegate(document, '.notification-shelf .js-notification-action button', 'click', handleClick);
}

[fregante]

SessionStorage is shared across tabs, so it should exist for as little as possible to avoid conflicts. To do this, set it onbeforeunload and read+unset it in init

[FloEdelmann]

SessionStorage is shared across tabs

No, it's not. https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage:

Opening a page in a new tab or window creates a new session with the value of the top-level browsing context, which differs from how session cookies work.

I don't think we need to delete the session storage item at all, because it will only exist in this tab; and when opening another notification, its init function will set it again to a new value.

Also, when clicking on the same notification bar multiple times, it should not behave differently. In other words; don't delete sessionStorage.rghIsNewTab; when clicking on "Done" in a new tab, because a subsequent click on "Move to inbox" would redirect to notifications page again.

[fregante]

No, it's not. developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage:

Indeed! Thanks for checking. I was confusing it with localStorage

[FloEdelmann]

@karlhorky Would you open a PR with your suggested changes, or do you want me to do it?

[karlhorky]

If this code is acceptable, happy to open a PR. How does this look? #3189