Opened 11 years ago
Closed 11 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 11 years ago by
Attachment: | 11567.html added |
---|
comment:1 Changed 11 years ago by
Component: | General → UI : Widgets |
---|---|
Description: | modified (diff) |
Milestone: | → CKEditor 4.4 |
Status: | new → confirmed |
Version: | → 4.3 |
comment:2 Changed 11 years ago by
comment:3 Changed 11 years ago by
Owner: | set to Piotr Jasiun |
---|---|
Status: | confirmed → assigned |
comment:4 Changed 11 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 11 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.html
use simplefilter.allow
and remove empty line. - in
widget/acf.html
define widget button instead of extracting allowed content from its definition.
comment:6 Changed 11 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.html
use simplefilter.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 11 years ago by
Status: | review → review_failed |
---|
There are some problems with pushed branches. Please make sure that everything is pushed.
comment:8 Changed 11 years ago by
Status: | review_failed → review |
---|
Everything is in the t/11567b branchs (both dev and tests).
comment:9 Changed 11 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 11 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
- git:aaddf43
- tests:16e1848
cc