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.

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] web_editor: prevent insertion of link on ctrl+k with video focused #162615

Open
wants to merge 1 commit into
base: 15.0
Choose a base branch
from

Conversation

perm-odoo
Copy link

This PR addresses an issue where selecting a video and pressing Ctrl+K would introduce a link(anchor tag) on top of the video. The fix ensures that pressing Ctrl+K while a video is focused will no longer insert a link, resolving the issue of an empty link being added.

task-2819776

@robodoo
Copy link
Contributor

robodoo commented Apr 19, 2024

@C3POdoo C3POdoo added the RD research & development, internal work label Apr 19, 2024
@perm-odoo perm-odoo force-pushed the 15.0-fix-prevent-link-on-ctrlk-for-video-perm branch from f46d835 to 38703bf Compare April 24, 2024 06:13
@msh-odoo msh-odoo force-pushed the 15.0-fix-prevent-link-on-ctrlk-for-video-perm branch from 38703bf to 9b21d5b Compare April 29, 2024 11:46
@msh-odoo msh-odoo marked this pull request as ready for review April 29, 2024 11:48
@C3POdoo C3POdoo requested a review from a team April 29, 2024 11:50
@qsm-odoo
Copy link
Contributor

qsm-odoo commented May 2, 2024

@robodoo delegate=robinlej
@robinlej

Copy link
Contributor

@robinlej robinlej left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Unfortunately, I think there is more digging to do.

Additionnally, your commit is signed <Odoo>. You should set up your git config properly so that the signature is your own (ideally <Firstname Lastname (quadrigram)>). :)

@@ -1125,6 +1125,10 @@ const Wysiwyg = Widget.extend({
core.bus.trigger('activate_image_link_tool');
return;
}
// Avoid invoking toggleLinkTools for videos.
if (targetEl.parentNode.closest(".media_iframe_video")) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like it works, but I'm afraid there is more to it than that.
If you:

  • insert a video,
  • click on it,
  • Ctrl + A,
  • Ctrl + K
    => it breaks the layout (the iframe's height is resized) and the DOM (a link tag is added and set around the whole video block structure, and the iframe is duplicated, once within the link tag, once outside of it).
    You can then just click on the buggy iframe to reset it to fullsize, but you end up with 2 iframes in the DOM (you only see one because they're both positioned absolutely).

Copy link
Contributor

@robinlej robinlej May 2, 2024

Choose a reason for hiding this comment

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

Slightly different way to break things:

  • Drop a text block
  • Select all the paragraphs, and type /video to add one
  • Put your cursor on the text block (e.g. in the empty paragraph that should still be there under the video)
  • Ctrl + A, then Ctrl + K: you can add a link (and have a visual bug in passing)
  • Save
    => There are 2 iframes, this time both visible, and potentially a link only accessible if you click on the empty paragraph.

Note that I don't think preventing Ctrl + A would be the right solution: you might want to do it and then Delete to remove the video. (But ideally, Ctrl + A should select every editable content within the <section>, like what happens when you do it from inside a paragraph. Probably not for this task, though, unless it helps solve the Ctrl+K bug.)

@perm-odoo perm-odoo force-pushed the 15.0-fix-prevent-link-on-ctrlk-for-video-perm branch 3 times, most recently from 47573dc to 1a8db45 Compare May 16, 2024 12:17
@perm-odoo perm-odoo force-pushed the 15.0-fix-prevent-link-on-ctrlk-for-video-perm branch from 1a8db45 to 44091e9 Compare June 4, 2024 07:02
This commit addresses an issue where selecting a video and pressing
Ctrl+K would introduce a link(anchor tag) on top of the video. Also if
the selection is section on Ctrl+A and Ctrl+K than whole section gets
inside link. The fix ensures that pressing Ctrl+K while a video is
focused will no longer insert a link and if section is in selection
than avoid toggling link tools.

task-2819776
@perm-odoo perm-odoo force-pushed the 15.0-fix-prevent-link-on-ctrlk-for-video-perm branch from 44091e9 to 131ebac Compare June 5, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants