1331638 - twitter.com broken if dom.webnotifications.enabled=false (due to faulty feature detection)

Status: RESOLVED FIXED

(Web Compatibility :: Site Reports, defect)

Description

[u580221]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170116030326

Steps to reproduce:

Retweet button on Twitter is broken (and so are many of the other UI buttons).

1. I visited https://twitter.com/doctorow/status/821069543653122049
2. I pressed the retweet button (below the image, the button with the two arrows pointing at their own tails making a circle)



Actual results:

Nothing happens, no login/sign up pop up, nothing.
Console shows error: ReferenceError: Notification is not defined


Expected results:

Retweet popup comes up.

Comment 1

[u580221]

This about:config change of mine might be related:

dom.webnotifications.enabled false

However, I don't want to enable it because I disabled it since so many modern web forums annoyed me with popups about it asking me to allow them to do notifications, and I can only click "Never" so often before I get fed up with this "feature".

Maybe there needs to be a new way of disabling web notifications which doesn't break the site (e.g. the API is still there but does nothing useful without the site necessarily realizing)?

Comment 2

[George Veneel Dogga [::TheLayman]]

Even with the config set to dom.webnotifications.enabled false, It shows me the login/signup pop up.

Comment 3

[Loic]

It seems to be a duplicate of bug 1328544.

Could you test:

1) in safe mode:
https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode

2) with a fresh profile:
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

Comment 4

[George Veneel Dogga [::TheLayman]]

(In reply to Loic from comment #3)
> It seems to be a duplicate of bug 1328544.
> 
> Could you test:
> 
> 1) in safe mode:
> https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-
> mode
> 
> 2) with a fresh profile:
> https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-
> firefox-profiles

Hey, I just tried them. Still the same. Unable to reproduce it.

Comment 5

[u580221]

Attachment: about_support.txt

As requested, I did some more thorough tests:

Regular firefox, dom.webnotifications.enabled false: retweet button broken (does nothing)
Regular firefox, dom.webnotifications.enabled true: retweet button works
firefox -safe-mode, dom.webnotifications.enabled false: retweet button broken (does nothing)
firefox -safe-mode, dom.webnotifications.enabled true: retweet button works
firefox with new profile (created through -P), dom.webnotifications.enabled false: retweet button works
firefox with new profile (created through -P), dom.webnotifications.enabled true: retweet button works

I attached the "Copy raw data to clipboard" output of about:support in about_support.txt of the affected profile. Hopefully, this will give someone a clue on what else is involved.

Comment 6

[Loic]

I confirm that dom.webnotifications.enabled=false breaks the call of the pop-up to confirm the retweet.
I tested FF48/50/53.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Isn't this a webcompat bug about twitter not feature detecting properly?

Comment 8

[Mike Taylor [:miketaylr]]

With dom.webnotifications.enabled=false, basically all of twitter is broken -- my timeline loads like 2 tweets before it fails.

In https://abs.twimg.com/c/swift/en/bundle/boot.8c42ae86d9bcfe0dd2dad0617d1f2712885114ba.js

this.isSupported = function() {
  var a = "PushManager" in window,
      b = this.getNotificationPermission() !== "denied",
      c = "serviceWorker" in navigator,
      d = c && "showNotification" in ServiceWorkerRegistration.prototype;
  return d && a && b && c
}

then

this.getNotificationPermission = function() {
    return Notification && Notification.permission
}

is what causes the "ReferenceError: Notification is not defined" error.

If that's rewritten as

this.getNotificationPermission = function() {
  return window.Notification && window.Notification.permission
}

...it will return undefined, and then the return value of `isSupported` will be false, presumably the rest will work after that. 

Even though they're trying to feature detect, they're doing so in a way that will throw.

Adam, would you mind letting Twitter know via our mailing list? Thanks!

Comment 9

[Mike Taylor [:miketaylr]]

(or they could go for something like:

this.getNotificationPermission = function() {
  return "Notification" in window && window.Notification.permission
}

since they are using that style for PushManager)

Comment 10

[u580221]

Don't you think maybe firefox should allow a way to disable web notifications without actually removing the API? (so that the page doesn't know it is disabled)

I can very easily imagine the same problem on other sites, given that probably all major browsers support it now so that some web developers will incorrectly assume it is always available.

Comment 11

[Masatoshi Kimura [:emk]]

If they assume it is always available, why they write a feature detection in the first place?

Comment 12

[u580221]

I would assume because all their test setups with major browsers have it, so nobody caught the mistake. And this is somewhat bound to happen to others too, isn't it?

Comment 13

[Mike Taylor [:miketaylr]]

(In reply to jonas from comment #10)
> Don't you think maybe firefox should allow a way to disable web
> notifications without actually removing the API? (so that the page doesn't
> know it is disabled)

I think that would probably cause more problems than it would solve. There's a ton of code out there that assumes if the API exists, support exists:

https://github.com/marcopaz/is-service-up/blob/c4d233dfa01dfdd9aa7f150d062473c192165d54/frontend/src/utils/notifications.js#L2

> 
> I can very easily imagine the same problem on other sites, given that
> probably all major browsers support it now so that some web developers will
> incorrectly assume it is always available.

Feature detection is a pretty well-known tool for developers these days. It's possible we'll get into trouble if devs make support assumptions based on UA string, or if they don't actually test their feature detection code in older browsers (or browsers w/ features disabled). But those problems aren't new to the Notifications API.

Comment 14

[Adam Stevenson [:adamopenweb]]

Sent them the details via the mailing list. Setting to sitewait.

Comment 15

[Adam Stevenson [:adamopenweb]]

We received a response:
"The fix is already in and should go out today or Monday"

Comment 16

[Adam Stevenson [:adamopenweb]]

This looks to be fixed now. Tested in Firefox 54 on OSX.
Thanks everyone!