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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: jonas, Assigned: adamopenweb)

Tracking

Firefox 53

Firefox Tracking Flags

(platform-rel +)

Details

(Whiteboard: [platform-rel-Twitter])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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)?
Even with the config set to dom.webnotifications.enabled false, It shows me the login/signup pop up.

Comment 3

2 years ago
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
Flags: needinfo?(jonas)
(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.
(Reporter)

Comment 5

2 years ago
Posted file 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.
Flags: needinfo?(jonas)

Comment 6

2 years ago
I confirm that dom.webnotifications.enabled=false breaks the call of the pop-up to confirm the retweet.
I tested FF48/50/53.
Component: Untriaged → DOM
Product: Firefox → Core
Summary: Retweet button on Twitter is broken (and so are many of the other UI buttons) → Retweet button on Twitter is broken (and so are many of the other UI buttons) with dom.webnotifications.enabled=false
Status: UNCONFIRMED → NEW
Ever confirmed: true
Isn't this a webcompat bug about twitter not feature detecting properly?
Flags: needinfo?(miket)
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!
Component: DOM → Desktop
Flags: needinfo?(miket) → needinfo?(astevenson)
Product: Core → Tech Evangelism
Whiteboard: [contactready]
Version: 53 Branch → Firefox 53
Summary: Retweet button on Twitter is broken (and so are many of the other UI buttons) with dom.webnotifications.enabled=false → twitter.com broken if dom.webnotifications.enabled=false (due to faulty feature detection)
(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)
(Reporter)

Comment 10

2 years ago
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.
If they assume it is always available, why they write a feature detection in the first place?
(Reporter)

Comment 12

2 years ago
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?
(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.
(Assignee)

Comment 14

2 years ago
Sent them the details via the mailing list. Setting to sitewait.
Assignee: nobody → astevenson
Status: NEW → ASSIGNED
Flags: needinfo?(astevenson)
Whiteboard: [contactready] → [sitewait]
platform-rel: --- → ?
Whiteboard: [sitewait] → [sitewait][platform-rel-Twitter]
(Assignee)

Comment 15

2 years ago
We received a response:
"The fix is already in and should go out today or Monday"
platform-rel: ? → +
(Assignee)

Comment 16

2 years ago
This looks to be fixed now. Tested in Firefox 54 on OSX.
Thanks everyone!
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Whiteboard: [sitewait][platform-rel-Twitter] → [platform-rel-Twitter]
Component: Desktop → Desktop
Product: Tech Evangelism → Web Compatibility
You need to log in before you can comment on or make changes to this bug.