Opened 6 years ago

Closed 5 years ago

#11133 closed Bug (fixed)

Pagebreak loses contenteditable=false when pasting

Reported by: Piotrek Koszuliński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.3.2
Component: General Version: 4.1 RC
Keywords: Cc:

Description

It's lost, because it's not part of pagebreak's ACF rules. It could be added by dataFilter rule, but the style checks do not pass on internal form of pagebreak (no display:none style).

BTW. Regexp patterns are compiled every time div is filtered - they should be cached.

Change History (10)

comment:1 Changed 6 years ago by Jakub Ś

Status: newconfirmed

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

Milestone: CKEditor 4.3.1CKEditor 4.3.2

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

Owner: set to Olek Nowodziński
Status: confirmedassigned

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

Status: assignedreview

Solution in t/11133 (+ corresponding test branch).

Still I'm not quite convinced whether such a weak condition is enough to distinguish pagebreaks from regular divs while pasting.

TODO: Pagebreak should definitely be a widget.

comment:5 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_failed

There's no such thing as element.classes. It's a leftover from ACF. Use element.hasClass.

And yes - pagebreak should be a widget.

comment:6 in reply to:  5 Changed 5 years ago by Olek Nowodziński

Status: review_failedreview

Replying to Reinmar:

There's no such thing as element.classes. It's a leftover from ACF. Use element.hasClass.

That's true, element.hasClass is the proper one. But it was more than that: since I checked element#classes for cke_pagebreak which was stripped out by ACF, the condition was also logically invalid. The correct check is

if ( element.attributes[ 'data-cke-display-name' ] )
    ...

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

Status: reviewreview_failed

Hm... this checks something totally different. This attribute carries information for elements path and can be used by various features. So this condition will give false positives.

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

Status: review_failedreview

It turned out that data-cke-pagebreak survives filtering and it's enough to tell pagebreak from other elements.

I also unified the way of setting the attributes of the "internal form" because I found it quite inconsistent and misleading. At the moment both pagebreak command and dataFilter rule use the same set of attributes generated by attributesSet which finally makes them totally coherent.

comment:9 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_passed

I pushed two commits to dev and tests. Solution is clean so it's good. Internal form is marked with a data-cke-pagebreak attribute and all data-cke-* attrs are allowed by ACF on input and are stripped by default data processor on output.

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

Resolution: fixed
Status: review_passedclosed

git:77ab1b8 (dev) and 00a8345 (tests) landed in master.

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