Opened 11 years ago

Closed 11 years ago

#11533 closed Bug (fixed)

Elements upcasted repeatedly if pseudo-DOM modified during upcast

Reported by: Olek Nowodziński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.3.3
Component: General Version: 4.3
Keywords: Cc:

Description

It turned out that in some cases upcasting may go out of control.

Assumptions:

  1. Let's start with the following html: 
    <body><p>x<img/></p></body>
  2. Each <img> is to be upcasted to <figure><img></figure>.

Now let's follow forEach, which ​calls the iterator (see: createUpcastIterator) during the upcast process, step-by-step:

  1. ForEach: Visit <p>.
  2. ForEach: Visit children of <p>.
  3. ForEach: "x" text found
  4. Upcast: Ignore text.
  5. ForEach: Visit <img>
  6. Upcast: Start upcasting <img> to <figure>.
  7. Upcast: Split <p> because <figure> is block. 
    <body><p>x</p><p><img/></p></body>
  8. Upcast: Replace the parent of <img> (<p>) with <figure> 
    <body><p>x</p><figure><img/></figure></body>
  9. Widget system: Remember that <figure> is to be wrapped. Great. But problems start now.
  10. ForEach: No more children of <p>.
  11. ForEach: Go back to <body>.
  12. ForEach: Move to <figure>, which is the next child of <body>
  13. ForEach: Enter <figure> (it's not a widget yet).
  14. ForEach: Visit children of <figure>.
  15. ForEach: Visit <img>.
  16. Upcast: Start upcasting <img>.
  17. Bang! Upcasting the same <img> once again, while it belongs to <figure> which is to be a widget.

Change History (8)

comment:1 Changed 11 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: new → assigned

comment:2 Changed 11 years ago by Piotrek Koszuliński

cc

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

Fix pushed to ​t/11533 (+tests).

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

Status: assigned → review

comment:5 Changed 11 years ago by Olek Nowodziński

Status: review → review_failed

Self criticism.

comment:6 Changed 11 years ago by Olek Nowodziński

Status: review_failed → review
  1. I updated other Widget/Image2 test to the new data-cke-widget-upcasted attribute API.
  2. I improved the fix to avoid infinite loop when <img> is replaced by <figure><img></figure> during upcast but additionally <figure> can be upcasted.

comment:7 Changed 11 years ago by Piotrek Koszuliński

Status: review → review_passed

Code looks good, tests pass, widgets work correctly and the patch is clear.

I was thinking about removing data-cke-widget-upcasted in htmlFilter (it's applied just after upcasting), so we would avoid any theoretically possible problems with handling widget which already was upcasted (e.g. pasting a widget), but at the same time this would affect performance, so let's wait and see if any real issues will appear.

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

Resolution: → fixed
Status: review_passed → closed

​git:fff2039 landed in master (17549d9 tests).

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy