Opened 6 years ago

Closed 5 years ago

#10884 closed Bug (fixed)

Widgets integration with showblocks

Reported by: Piotrek Koszuliński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.3.2
Component: General Version: 4.3 Beta
Keywords: Cc: joel.peltonen@…

Description

Widget wrapper is now displayed by showblocks. We can do nothing about this or hide its outline.

Attachments (2)

10884.html (1.4 KB) - added by Olek Nowodziński 5 years ago.
A simple playground
10884_2.html (1.5 KB) - added by Piotrek Koszuliński 5 years ago.

Download all attachments as: .zip

Change History (18)

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

Status: newconfirmed

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

Milestone: CKEditor 4.3CKEditor 4.3.1

comment:3 Changed 6 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

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

Milestone: CKEditor 4.3.1CKEditor 4.3.2
Owner: Marek Lewandowski deleted
Status: assignedconfirmed

comment:5 Changed 6 years ago by Joel

Cc: joel.peltonen@… added

Add CC, this messes with my usage.

comment:6 Changed 5 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:7 Changed 5 years ago by Marek Lewandowski

Simplified solution pushed to t/10884 which removes borders only from widget wrappers.

  1. I proposed pluginIntegrations variable where we might group integration functions. I'd like to see contextmenu integration there as well.
  2. I have not used editor variable in pluginIntegrations.showblocks intentionally, but i passed it within for loop, coz other integrations will need it.

We might discuss here about improvement of this integration, because we've been talking a little bit with guys - that most annoying/distracting may become borders for markup inside widget template, which is not editable itself.

We've been considering CSS selectors using :not() pseudoselector, but it's not supported by IE8. One IE8 friendly solution might be CSS like:

.cke_show_blocks div.cke_widget_wrapper, .cke_show_blocks div.cke_widget_wrapper div, .cke_show_blocks div.cke_widget_wrapper p {
    border: none;
    padding-top: 0px;
	background-image: none;
	padding-left: 0px;
}

.cke_show_blocks div.cke_widget_wrapper .cke_widget_editable div, .cke_show_blocks div.cke_widget_wrapper .cke_widget_editable p {
    background-repeat: no-repeat;
    border: 1px dotted gray;
    padding-top: 8px;
	padding-left: 8px;
}

.cke_show_blocks div.cke_widget_wrapper .cke_widget_editable div {
    background-image: url(http://cke.alt/plugins/showblocks/images/block_div.png);
}

.cke_show_blocks div.cke_widget_wrapper .cke_widget_editable p {
    background-image: url(http://cke.alt/plugins/showblocks/images/block_p.png);
}

Note that there are only p and div tags are covered in sample above, assuming ltr dir, for simplicity sake.


Along with that i suggest to include figure and figcaption, but it may make sense to already include all new html5 block tags to have smoother html5 transition.

comment:8 Changed 5 years ago by Marek Lewandowski

Status: assignedreview

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

Status: reviewreview_failed

That "pluginIntegrations thing" is not a good idea really. It's not extendable and 3rd party developers will not be able to make any use of it. Let's KISS first.

This is not the only problem though. I researched quite a lot and figured out that with current state of CSS we're able to disable showblock styles for wrapper only. Of course, our intention is to have showblock styles disabled for non-editable widget internals (descendants of wrapper but ascendants of nested editables) but there's no way to make such selection with CSS. Even if we decided to disable showblock styles for all descendants of widget wrapper, it's still impossible because by specifying such rule, we overwrite existing styles (i.e. forcing border:none for all children of .cke_widget_wrapper will distort the natural look of <figure> in Image2 and so on).

I'm for the simplest solution which is to disable showblock styles for wrapper only but in Showblocks plugin (which should take care of itself) instead of Widget plugin.

P.S.: PK suggested a `:read-write` CSS pseudo-class but it does not seem to work and it's not widely supported anyway.

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

Owner: changed from Marek Lewandowski to Olek Nowodziński
Status: review_failedassigned

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

P.S.: PK suggested a ​:read-write CSS pseudo-class but it does not seem to work and it's not widely supported anyway.

I suggested checking if it may become a solution one day. Sure, it cannot be used now, but if it's promising, we can pick biggest KISS for now and wait until more browsers will have good implementation of that pseudoclass.

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

Status: assignedreview

Proposed solution pushed to t/10884b. I also re-factorized Showblocks plugin to save some bytes and make the code maintainable.

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

Status: reviewreview_failed

Showblocks should not use cke_widget_wrapper class. If we need class we should introduce new class like cke_showblocks_off and use it in widgets. Or use simple [contenteditable=false] selector.

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

Status: review_failedreview

I updated t/10884b with a patch using [contenteditable=false].

Changed 5 years ago by Olek Nowodziński

Attachment: 10884.html added

A simple playground

Changed 5 years ago by Piotrek Koszuliński

Attachment: 10884_2.html added

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

Status: reviewreview_passed

While reviewing this ticket an idea came to my mind. We were trying to handle disabling showblocks in widgets automatically. This turned out to be impossible because of CSS limitations. The only automatic CSS-based solution would have to use :read-write pseudoclass, so it's not yet possible, because none of the browsers support it.

So, instead of automatic handling we can give developers an easy option to disable showblocks on elements inside their widgets. It will of course require additional (but small) effort from developer, but it also assure that it's up to developer which elements should not be marked by showblocks. There may be e.g. cases when widget implements contains an HTML structure which is significant from semantical POV and could be showed.

I pushed additional commits to t/10884b which:

  • makes all browser except IE8 using :not() to exclude elements with contenteditable=false or cke_show_blocks_off class (on IE8 those elements cannot be excluded so we simply reset showblocks styles for them - a little bit worse solution, but there's no better option),
  • fixes leaking JS variables.

You can see in 10884_2.html how cke_show_blocks_off class disables showblocks. In real world cases it should be added in widget's upcast method and removed while downcasting, so it doesn't leak to data.

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

Resolution: fixed
Status: review_passedclosed

git:8c72ce0 landed in master.

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