Opened 12 years ago
Closed 12 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 )
See 11567.html. Image should be filtered out because there's no image button in the toolbar.
Attachments (1)
Change History (11)
Changed 12 years ago by
| Attachment: | 11567.html added |
|---|
comment:1 Changed 12 years ago by
| Component: | General → UI : Widgets |
|---|---|
| Description: | modified (diff) |
| Milestone: | → CKEditor 4.4 |
| Status: | new → confirmed |
| Version: | → 4.3 |
comment:2 Changed 12 years ago by
comment:3 Changed 12 years ago by
| Owner: | set to Piotr Jasiun |
|---|---|
| Status: | confirmed → assigned |
comment:4 Changed 12 years ago by
| Status: | assigned → review |
|---|
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 12 years ago by
| Status: | review → review_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.htmluse simplefilter.allowand remove empty line. - in
widget/acf.htmldefine widget button instead of extracting allowed content from its definition.
comment:6 Changed 12 years ago by
| Status: | review_failed → review |
|---|
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.htmluse simplefilter.allowand remove empty line.- in
widget/acf.htmldefine 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 12 years ago by
| Status: | review → review_failed |
|---|
There are some problems with pushed branches. Please make sure that everything is pushed.
comment:8 Changed 12 years ago by
| Status: | review_failed → review |
|---|
Everything is in the t/11567b branchs (both dev and tests).
comment:9 Changed 12 years ago by
| Status: | review → review_passed |
|---|
R+. Please remember about adding appropriate information about this change in the "Important Notes" section in the changelog.
comment:10 Changed 12 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | review_passed → closed |
- git:aaddf43
- tests:16e1848

cc