When external links set to open in new tab, link cannot be clicked twice


(Matches) #1

Expected: If I click on a hyperlink, my browser treats it as a link. If I click a link and it opens a new tab, it will continue to open a new tab, and go to the link that I clicked.

Actual: The link href is removed and no longer works.


Clicking an attachment link works only once
(Michael Downey) #2

FYI, this appears to only happen for logged-in users with the default external links in new tab set to true. If a user is not logged in, can’t reproduce.


(lid) #3

Interesting bug.

https://github.com/discourse/discourse/blob/master/app/assets/javascripts/discourse/views/topic.js.es6#L45-L54

The click event for links in topics is handled with a mouseup event.

The ClickTrack event handler Can not cancel the click event using e.preventdefualt. And instead uses a Hackish fix to prevent the click by removing the ‘href’ attribute.

It will not reflect when

  • The user uses CTRL+LMB the method will exit before the href removal

  • The preference for external links in a new tab is off. navigating back will refresh the dom.

For this bug to work “default external links in new tab set to true” ( thanks @downey )
When a regular left click is triggered LMB in combine with the open in new tab.
Then the href attribute removal is showing it’s teeth.

Possible solutions:

####Solution1

To make e.preventDefault(); work inside the Discourse.ClickTrack.trackClick
I believe we will need to change the event handler from ‘mouseup’ to ‘click’ binding.
so the event will be cancelable without removing the href

####Solution 2 - ugly
a hack fix will require another hack fix to be fixed.

https://github.com/discourse/discourse/blob/master/app/assets/javascripts/discourse/lib/click_track.js#L86-L92

//add before line 89
//restore href attribute after tab open and click even finished time based
var orig_href = $link.attr('href');
      Em.run.later(function (){$link.attr('href',orig_href );},400);

(loopback0 - TDWTF) #4

Solution 3
Stop screwing with mouse clicks on links and let the browser worry about them entirely.


(Sam Saffron) #5

Disabling all the link tracking stuff should be a discourse admin function (if its not, we would be more than happy for a PR)

Now, if you want the link tracking stuff, you are going to have to be doing some fancy, no real other way around it.


(loopback0 - TDWTF) #6

If it should be an admin function, then why should the community submit a PR rather than the actual dev team doing it?

I don’t but that’s besides the point - why does it effectively disable a link after the first click? I’m pretty sure it didn’t behave like that originally.


(Matches) #7

This is new behavior.


(Sam Saffron) #8

We have zero paying customers asking for it. It is not a priority. We will eventually get to it.

This is a bug, the OP is 100% as legit bug we will get fixed … not following the need for drama here.


(Jeff Atwood) #9

If it is a regression, though, we should try to figure out which commit regressed it.


(Matches) #10

It’s not a regression since this has never happened in the past. It is a brand new bug within the last few days.


(Jeff Atwood) #11

Right, what I’m saying is, this used to work. In that case we should try to un-regress it.


(loopback0 - TDWTF) #12

There wasn’t a reply suggesting it was a legit bug, just the one I quoted which didn’t suggest that at all.
Had it done so, I’d not have replied like that. Now it’s been acknowledged as a legit bug, I’ll leave you guys to it.


(lid) #13

Should do the trick
https://github.com/discourse/discourse/pull/2869/


####Edit: I wonder why this proper fix PR went unnoticed and completely ignored. you preferred to go with my secondary solution that I I clearly said it is an ugly fix to another uglier fix. and use a timeout to restore the href. that should not have been removed in the first place.


(Sam Saffron) #14

I fixed this, but worry a bit about the long term health of that code


(Sam Saffron) #15