Opened 4 years ago

Closed 3 years ago

#13420 closed Bug (fixed)

Autoembed's rule which checks whether link's href equals links content should take encoding into consideration

Reported by: Piotrek Koszuliński Owned by: Szymon Cofalik
Priority: Normal Milestone: CKEditor 4.5.2
Component: General Version: 4.5.0 Beta
Keywords: Cc:

Description

  1. Open http://tests.ckeditor.dev:1030/tests/plugins/autoembed/manual/autoembed
  2. Paste: https://www.youtube.com/watch?v=GUl9_5kK9ts&x=1

Result: it does not trigger autoembedding. That's because of the "&" character which is encoded in text node, but it isn't encoded in link.attributes.href.

Change History (9)

comment:1 Changed 3 years ago by Szymon Cofalik

Owner: set to Szymon Cofalik
Status: newassigned

comment:2 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

Fixed on branch:t/13420.

I changed how links are decided to be autoembedded or not. The requirement that has not been met in bugged version is that the link has to have same href and inner text. So I changed the regexp to check if such link was pasted.

Previously, it was done through htmlParser which messes too much with what was pasted. I tried to toy with encodeURIComponent() and try to tweak previous solution but the code got too messy and there was a few cases to think about. Now it's clean and works fine.

comment:3 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_failed

I rebased the branch on master and pushed a commit git:5aea655 with a test which refers to #13419. Unfortunately, since #13419, we should not assume that href corresponds with innerHTML for true embeddables. There got to be smarter than that.

Last edited 3 years ago by Olek Nowodziński (previous) (diff)

comment:4 Changed 3 years ago by Olek Nowodziński

Idea: what if we used decodeURI() to normalise href and then compare to innerHTML?

comment:5 Changed 3 years ago by Szymon Cofalik

I am not sure how it will work with links that has already encoded chars in their text. Will have to check.

comment:6 Changed 3 years ago by Szymon Cofalik

Fixed on branch:t/13420.

I also pushed branch:t/13420b with a bit more expanded solution. This solution passes some more complicated tests. It also uses htmlentity decoding to change signs like & into & when matching if href equals contents.

Theoretically this may be a better solution, but those test cases are not reproducible manually, even if I write <a href="https://foo.bar/g/200/313?foo=%22æåãĂĄ&quot;">https://foo.bar/g/200/313?foo=&quot;æåãĂĄ"</a> in source, then copy this link and paste into editor - browser or CKE (not sure which one) takes care of those entities before we get the data in autoembed plugin. So these extra tests are only theoretical cases. It might be better to use this fix though if this behavior change somewhere in the future.

comment:7 Changed 3 years ago by Szymon Cofalik

Status: review_failedreview

comment:8 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_passed

branch:t/13420 is enough right now and I'm OK to close it.

comment:9 Changed 3 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Merged git:2b30328 into master. Good job!

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy