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

RESOLVED FIXED

Status

Webmaker
Thimble
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: pomax, Assigned: pomax)

Tracking

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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

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

Updated

5 years ago
Assignee: nobody → dchen.xd

Comment 2

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

Comment 3

5 years ago
Mike, any suggestion on which file I should start to working on?
Flags: needinfo?(pomax)
(Assignee)

Comment 4

5 years ago
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.
Flags: needinfo?(pomax)

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

4 years ago
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?
Flags: needinfo?(jon)

Comment 9

4 years ago
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?
Flags: needinfo?(jon)
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
Status: NEW → ASSIGNED
(Assignee)

Comment 11

4 years ago
worth changing.
Assignee: dchen.xd → pomax
Attachment #8361807 - Flags: review?(scott) → review+

Comment 13

4 years ago
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
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.