925446 - external links that are of the form href="//...." are not target="_blank" linked atm

Status: RESOLVED FIXED

(Webmaker Graveyard :: Thimble, defect)

Description

[Pomax]

example: https://artychan.makes.org/thimble/helping-to-make-a-million-mozillians, click on the "@artychan" link, which is an absolute href="//...." link. It will open in the shell iframe rather than as new/on top

Comment 1

[xdchen(:xdchen)]

Hi, if it's still available, I would like to take it. thank you

Comment 2

[xdchen(:xdchen)]

Mike, could you please give me some clue where to start it? thank you

Comment 3

[xdchen(:xdchen)]

Mike, any suggestion on which file I should start to working on?

Comment 4

[Pomax]

we have some code that is supposed to get injected into the final make in https://github.com/mozilla/thimble.webmaker.org/blob/master/lib/middleware.js#L339 and then used as https://github.com/mozilla/thimble.webmaker.org/blob/master/lib/middleware.js#L384

These things should already kick in, so it's worth seeing if, when publishing something, they actually happen or not.

Comment 5

[xdchen(:xdchen)]

make some trial publishing, the link opened in new window/tab, seems the bug is already solved.

Comment 6

[Jon Buckley [:jbuck]]

On the link that Pomax gave, the link doesn't open in a new window or tab

Comment 7

[xdchen(:xdchen)]

(In reply to Jon Buckley [:jbuck] from comment #6)
> On the link that Pomax gave, the link doesn't open in a new window or tab

Maybe the updated code not yet be built into the production release, while it do works in my local test environment.

Comment 8

[Pomax]

Given that the embed shell from make-valet now takes care of the framing, the link fix code will never trigger. Jbuck, is there any way to get this code added in the make-valet for thimble publications?

Comment 9

[Jon Buckley [:jbuck]]

What if during the publish process, the thimble page ran through the entire page, and rewrote the <a> tags to include the target? Would that be a better solution? Is there something that will let us iterate over all the html tags to do that?

Comment 10

[Jon Buckley [:jbuck]]

Actually, this has nothing to makevalet or thimble; the code only adds the target attribute, it's the code only working if the href has '://' in it, which arty's make does not. This is a one character patch on https://github.com/mozilla/thimble.webmaker.org/blob/8370653eba9e8ce0431b80cb31ee8f338f88c44d/lib/middleware.js#L342

Comment 11

[Pomax]

worth changing.

Comment 12

[Pomax]

Attachment: https://github.com/mozilla/thimble.webmaker.org/pull/345

Comment 13

[[github robot]]

Commit pushed to master at https://github.com/mozilla/thimble.webmaker.org

https://github.com/mozilla/thimble.webmaker.org/commit/c55f0abf32e3a18fceda13734add0920ee04bb49
Merge pull request #345 from Pomax/bug925446

better absolute link detection for opening links in new windows