Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix missing title attribute on <iframe> tag suggested for embedding #3901

Merged
merged 6 commits into from
Mar 31, 2021
Merged

fix missing title attribute on <iframe> tag suggested for embedding #3901

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
122b39b
title attribute is missing on <iframe> tag suggested for embedding #3861
pthavarasa Mar 29, 2021
71bd422
fix #3901
pthavarasa Mar 30, 2021
79b26dc
fix: escapeHTML #3901
pthavarasa Mar 30, 2021
74b2536
fix: playlist title instead of video title #3901
pthavarasa Mar 30, 2021
2f61b74
fix #3901
pthavarasa Mar 30, 2021
bca946a
assign title directly #3901
pthavarasa Mar 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion client/src/app/+admin/moderation/video-block-list/video-block-list.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@ export class VideoBlockListComponent extends RestTable implements OnInit, AfterV
baseUrl: `${environment.originServerUrl}/videos/embed/${entry.video.uuid}`,
title: false,
warningTitle: false
})
}),
entry.video.name
)
}

Expand Down
1 change: 1 addition & 0 deletions client/src/app/+videos/+video-watch/video-watch.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,7 @@ export class VideoWatchComponent implements OnInit, OnDestroy {
? this.videoService.getVideoViewUrl(video.uuid)
: null,
embedUrl: video.embedUrl,
embedTitle: video.name,

isLive: video.isLive,

Expand Down
3 changes: 2 additions & 1 deletion client/src/app/shared/shared-abuse-list/abuse-list-table.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ export class AbuseListTableComponent extends RestTable implements OnInit, AfterV
warningTitle: false,
startTime: abuse.video.startAt,
stopTime: abuse.video.endAt
})
}),
abuse.video.name
)
}

Expand Down
3 changes: 2 additions & 1 deletion client/src/app/shared/shared-moderation/report-modals/video-report.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ export class VideoReportComponent extends FormReactive implements OnInit {
baseUrl: this.video.embedUrl,
title: false,
warningTitle: false
})
}),
this.video.name
)
)
}
Expand Down
4 changes: 2 additions & 2 deletions client/src/app/shared/shared-share-modal/video-share.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,14 @@ export class VideoShareComponent {
const options = this.getVideoOptions(this.video.embedUrl)

const embedUrl = buildVideoLink(options)
return buildVideoOrPlaylistEmbed(embedUrl)
return buildVideoOrPlaylistEmbed(embedUrl, this.video.name)
}

getPlaylistIframeCode () {
const options = this.getPlaylistOptions(this.playlist.embedUrl)

const embedUrl = buildPlaylistLink(options)
return buildVideoOrPlaylistEmbed(embedUrl)
return buildVideoOrPlaylistEmbed(embedUrl, this.playlist.displayName)
}

getVideoUrl () {
Expand Down
9 changes: 5 additions & 4 deletions client/src/assets/player/peertube-player-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export interface CommonOptions extends CustomizationOptions {

videoViewUrl: string
embedUrl: string
embedTitle: string

isLive: boolean

Expand Down Expand Up @@ -165,7 +166,7 @@ export class PeertubePlayerManager {
PeertubePlayerManager.alreadyPlayed = true
})

self.addContextMenu(mode, player, options.common.embedUrl)
self.addContextMenu(mode, player, options.common.embedUrl, options.common.embedTitle)

player.bezels()

Expand Down Expand Up @@ -203,7 +204,7 @@ export class PeertubePlayerManager {
videojs(newVideoElement, videojsOptions, function (this: videojs.Player) {
const player = this

self.addContextMenu(mode, player, options.common.embedUrl)
self.addContextMenu(mode, player, options.common.embedUrl, options.common.embedTitle)

PeertubePlayerManager.onPlayerChange(player)
})
Expand Down Expand Up @@ -492,7 +493,7 @@ export class PeertubePlayerManager {
return children
}

private static addContextMenu (mode: PlayerMode, player: videojs.Player, videoEmbedUrl: string) {
private static addContextMenu (mode: PlayerMode, player: videojs.Player, videoEmbedUrl: string, videoEmbedTitle: string) {
const content = [
{
label: player.localize('Copy the video URL'),
Expand All @@ -509,7 +510,7 @@ export class PeertubePlayerManager {
{
label: player.localize('Copy embed code'),
listener: () => {
copyToClipboard(buildVideoOrPlaylistEmbed(videoEmbedUrl))
copyToClipboard(buildVideoOrPlaylistEmbed(videoEmbedUrl, videoEmbedTitle))
}
}
]
Expand Down
3 changes: 2 additions & 1 deletion client/src/assets/player/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,10 @@ function secondsToTime (seconds: number, full = false, symbol?: string) {
return time
}

function buildVideoOrPlaylistEmbed (embedUrl: string) {
function buildVideoOrPlaylistEmbed (embedUrl: string, embedTitle: string) {
return '<iframe width="560" height="315" ' +
'sandbox="allow-same-origin allow-scripts allow-popups" ' +
'title="' + embedTitle + '" ' +
'src="' + embedUrl + '" ' +
'frameborder="0" allowfullscreen>' +
'</iframe>'
Expand Down
3 changes: 2 additions & 1 deletion client/src/standalone/videos/embed.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,8 @@ export class PeerTubeEmbed {

serverUrl: window.location.origin,
language: navigator.language,
embedUrl: window.location.origin + videoInfo.embedPath
embedUrl: window.location.origin + videoInfo.embedPath,
embedTitle: videoInfo.name
},

webtorrent: {
Expand Down
3 changes: 2 additions & 1 deletion server/controllers/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ function buildOEmbed (options: {
const embedUrl = webserverUrl + embedPath
let embedWidth = EMBED_SIZE.width
let embedHeight = EMBED_SIZE.height
let embedtitle = title

let thumbnailUrl = previewPath
? webserverUrl + previewPath
Expand All @@ -96,7 +97,7 @@ function buildOEmbed (options: {
}

const html = `<iframe width="${embedWidth}" height="${embedHeight}" sandbox="allow-same-origin allow-scripts" ` +
`src="${embedUrl}" frameborder="0" allowfullscreen></iframe>`
`title="${embedtitle}" src="${embedUrl}" frameborder="0" allowfullscreen></iframe>`

const json: any = {
type: 'video',
Expand Down
6 changes: 3 additions & 3 deletions server/tests/api/server/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ describe('Test services', function () {

const res = await getOEmbed(server.url, oembedUrl)
const expectedHtml = '<iframe width="560" height="315" sandbox="allow-same-origin allow-scripts" ' +
`src="http://localhost:${server.port}/videos/embed/${video.uuid}" ` +
`title="${video.name}" src="http://localhost:${server.port}/videos/embed/${video.uuid}" ` +
'frameborder="0" allowfullscreen></iframe>'
const expectedThumbnailUrl = 'http://localhost:' + server.port + video.previewPath

Expand All @@ -88,7 +88,7 @@ describe('Test services', function () {

const res = await getOEmbed(server.url, oembedUrl)
const expectedHtml = '<iframe width="560" height="315" sandbox="allow-same-origin allow-scripts" ' +
`src="http://localhost:${server.port}/video-playlists/embed/${playlistUUID}" ` +
`title="${video.name}" src="http://localhost:${server.port}/video-playlists/embed/${playlistUUID}" ` +
Copy link
Owner

@Chocobozzz Chocobozzz Mar 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be title="The Life and Times of Scrooge McDuck" since it's the playlist embed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can i assign directly the string ?

'frameborder="0" allowfullscreen></iframe>'

expect(res.body.html).to.equal(expectedHtml)
Expand All @@ -109,7 +109,7 @@ describe('Test services', function () {

const res = await getOEmbed(server.url, oembedUrl, format, maxHeight, maxWidth)
const expectedHtml = '<iframe width="50" height="50" sandbox="allow-same-origin allow-scripts" ' +
`src="http://localhost:${server.port}/videos/embed/${video.uuid}" ` +
`title="${video.name}" src="http://localhost:${server.port}/videos/embed/${video.uuid}" ` +
'frameborder="0" allowfullscreen></iframe>'

expect(res.body.html).to.equal(expectedHtml)
Expand Down