Opened 10 years ago

Closed 10 years ago

#11567 closed Bug (fixed)

Widget definition without button should not be automatically registered to ACF

Reported by: Piotrek Koszuliński Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.4.0
Component: UI : Widgets Version: 4.3
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

See 11567.html. Image should be filtered out because there's no image button in the toolbar.

Attachments (1)

11567.html (740 bytes) - added by Piotrek Koszuliński 10 years ago.

Download all attachments as: .zip

Change History (11)

Changed 10 years ago by Piotrek Koszuliński

Attachment: 11567.html added

comment:1 Changed 10 years ago by Piotrek Koszuliński

Component: GeneralUI : Widgets
Description: modified (diff)
Milestone: CKEditor 4.4
Status: newconfirmed
Version: 4.3

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

cc

comment:3 Changed 10 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:4 Changed 10 years ago by Piotr Jasiun

Status: assignedreview

Fixed, but...

This is not the best way to fix this problem. Now, after fix, if you remove button, it is removed from ACF but the widget may still work (like placeholder widget which does not need any ACF rule), so removing button only partially remove widget. Also features depends no toolbar now, what seems to be a bad way.

Anyway such solution is good enough for now. To introduce better we would need big changed in our architecture.

Changes in t/11567 and corresponding test branch.

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

Status: reviewreview_failed

The only thing you need to on dev is removing the wrong code from widget plugin and that's all. Image2 adds button which uses a command which inherits requiredContent and allowedContent from a widget. This is totally automatic and you don't need this ugly hack in image2 plugin which you added in branch:t/11567. But I guess that you were mislead the fact that after removing Image2's button image wasn't fully removed. That's because of the smiley plugin, not because of lack synchronization between Image2 button and ACF.

Besides that:

  • You won't need those tests at all: 084803c.
  • In the commit 18f5868d you can:
    • remove all the unnecessary allowedContent definitions in magicline tests and just disable ACF; we're only testing, so we should keep this short,
    • in widget/nestededitables.html use simple filter.allow and remove empty line.
    • in widget/acf.html define widget button instead of extracting allowed content from its definition.

comment:6 Changed 10 years ago by Piotr Jasiun

Status: review_failedreview

I removed changes from Image2 and you are right: everything is fine. I only needed to add toolbar plugin to some of the Image2 tests. And yes, now I don't need 084803c tests.

  • in widget/nestededitables.html use simple filter.allow and remove empty line.
  • in widget/acf.html define widget button instead of extracting allowed content from its definition.

Done.

  • remove all the unnecessary allowedContent definitions in magicline tests and just disable ACF; we're only testing, so we should keep this short,

There is a comment in that test:

// Block for testing nested editables with
// a very strict allowedContent.

So disabling ACF might be not the best solution here. I only replaced addFeature with filter.allow.

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

Status: reviewreview_failed

There are some problems with pushed branches. Please make sure that everything is pushed.

comment:8 Changed 10 years ago by Piotr Jasiun

Status: review_failedreview

Everything is in the t/11567b branchs (both dev and tests).

Last edited 10 years ago by Piotr Jasiun (previous) (diff)

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

Status: reviewreview_passed

R+. Please remember about adding appropriate information about this change in the "Important Notes" section in the changelog.

comment:10 Changed 10 years ago by Piotr Jasiun

Resolution: fixed
Status: review_passedclosed
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