Opened 6 years ago

Closed 6 years ago

#9467 closed Bug (fixed)

BIDI: Indent & List icons do not always reflect the text direction selected in the editor

Reported by: Teresa Monahan Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.0
Component: Core : BiDi Version: 4.0
Keywords: IBM Cc: Damian, Satya Minnekanti

Description

To reproduce, open any sample and select the 'Text direction from right to left' toolbar icon.

Problem: The indent, outdent, bulleted list and numbered lists icons do not change. They should be reversed to reflect the new RTL text direction. Switching back to LTR text direction should return the icons to their original state.

This should work similarly when running the editor in an RTL language.

This is a regression in v4.0. It works correctly in 3.6.5.

Change History (20)

comment:1 Changed 6 years ago by Garry Yao

Component: GeneralCore : BiDi
Milestone: CKEditor 4.0
Status: newconfirmed

comment:2 Changed 6 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

Call for a review on t/9467 with the part of dev change to make it happen, release fix is supposed to be provided on builder side instead.

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

Owner: changed from Garry Yao to Piotrek Koszuliński
Status: reviewassigned

It should work in release version out of the box, but there's a small issue - compiled editor.css contains rules which overwrite inline styles which are set correctly (for LTR and RTL). So the question is - what are these rules in editor.css? Why do they double inline styles?

Last edited 6 years ago by Piotrek Koszuliński (previous) (diff)

comment:4 Changed 6 years ago by Wiktor Walc

@Reinmar - this is by design in release version. Icons are defined in CSS files.

When talking about the release version, the problem is that right now, when generating the CSS styles, the Builder is not taking into consideration such cases like described here.

The only CSS rules created for an ordered list are listed below:

.cke_button__numberedlist_icon {background:url(icons.png) no-repeat 0 -1152px!important}
.cke_rtl .cke_button__numberedlist_icon{background:url(icons.png) no-repeat 0 -1120px!important}

Obviously, we're missing here a rule that was hardcoded in icons.css in V3:

.cke_skin_kama .cke_mixed_dir_content .cke_button_numberedlist .cke_icon { (...)

V4

In order to apply the RTL icon in cke_ltr environment to selected icons, an additional class would be needed to identify the buttons where the icon should be switched. If we e.g. agree to add dynamically a cke_mixed_dir class to a button:

current code:

<a id="cke_47" class="cke_button cke_button__numberedlist (...)
    <span class="cke_button_icon cke_button__numberedlist_icon" (...)

expected code:

<a id="cke_47" class="cke_button cke_mixed_dir cke_button__numberedlist (...)
    <span class="cke_button_icon cke_button__numberedlist_icon" (...)

then, while creating the release version, we will be able to add programatically proper css rules for bidi environment in editor.css for all icons that have rtl&ltr versions:

.cke_button__numberedlist_icon {background:url(icons.png) no-repeat 0 -1152px!important}
.cke_mixed_dir .cke_button__numberedlist_icon {background:url(icons.png) no-repeat 0 -1120px!important}
.cke_rtl (...)
.cke_rtl .cke_mixed_dir (...)

(just a sample)

The little drawback of this approach is that in such CSS files, some CSS rules would not be used at all, because not every button will be contentDirAware and Builder will have no information about that (so it will create just a bit more CSS rules than needed).

comment:5 Changed 6 years ago by Garry Yao

Owner: changed from Piotrek Koszuliński to Garry Yao
Status: assignedreview

Ok, for the skin icons, following Wiktor's clarification of the builder output on the skin CSS, I think it's possible to not bother the builder - I've pushed one change on t/9467 with a minor "cke_rtl" environment selector to the button element, so it should work out of the box.

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

Status: reviewreview_failed

This won't work in RTL editor with LTR part of content. Standard RTL rule has higher priority.

comment:7 in reply to:  6 Changed 6 years ago by Garry Yao

Status: review_failedreview

Replying to Reinmar:

This won't work in RTL editor with LTR part of content. Standard RTL rule has higher priority.

Tks for the catch, I've revised it with a commit.

comment:8 Changed 6 years ago by Garry Yao

For the plugins icons (sprites) in released copy though, ckbuilder need to be fixed, it currently append a chunk of code to the core file (ckeditor.js) that manipulates the icon definitions, but unfortunately RTL version is missing, so I've opened t/9467 on ckbuilder repo to start making this change.

@Wiktor, can u check this change and eventually make a update to the builder for that?

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

One more thing, about which I didn't know yesterday, may need clarification. CSS rules now double inline styles, because this way we handle skins that may not contain all icons for plugins. Plugin defines its own icon which is set in button's inline style. Builder gathers all icons in skin/icons directory, compiles them to the strip and creates CSS rules overwriting inline styles.

In this scenario we need to handle icons in JS and CSS at the same time, because JS don't know anything about CSS and vice versa. It looks like a waste for me. Especially when we consider mixed_dir which forces us to create more and more CSS rules. This shows that the logic shouldn't be kept in CSS :).

Thus, I propose to make few changes in current infrastructure - skin should define list of icons in JS (string 'name1,name2,name3' created by skin author). These icons should be added at startup to CKEDITOR.skin.icons (e.g. by CKEDITOR.skin.addIcon). Then plugins will add their icons, but only if skin icon doesn't exist (logic handled by addIcon()). And the rest will be handled by the code we already have. For release version we need builder creating CKEDITOR.skin.icons object (as a replacement of icons list in string), rather than CSS rules and again attaching it with higher priority than plugins' icons.

This approach has in my opinion advantage - we save few kilobytes (no long and doubling CSS rules), it's simpler and more flexible in the future. Imagine e.g. we'd have to add another set of selectors for new combination of mixed, rtl, ltr, whatever :)

If you agree with my idea, but you don't agree to work on this now I'll open a new ticket. However, bear in mind that we'll have second testing phase for skin, so it may include also this subject and that this approach should be implementable in less than one day.

comment:10 in reply to:  9 Changed 6 years ago by Frederico Caldeira Knabben

Replying to Reinmar:

As you can see in the source version, we're doing exactly what you're saying - using CKEDITOR.skin.icons everywhere. It is logic.

Our problem is with the release versions of both CKEditor and skins (note the plural).

In the release, ckeditor.js contains all plugins icons configured with CKEDITOR.skin.icons. ckeditor.js also contains the release code of skin.js. Because of this, skin.js is not needed in the skin folder.

You're saying that we could also have, inside ckeditor.js, the skin icons configured with CKEDITOR.skin.icons as well. This would be ok… IF it would never be needed to change the editor skin.

The problem is that skins are not in any way required to provide icons now. All of them come from plugins. But still, the skin can define an *unknown* set of icons, overriding the plugin ones.

So, let's supposed that we have a release version of CKEditor with 20 icons provided by plugins and 15 icons provided by the skin included in the build. We could have the 15 skin icons hardcoded in ckeditor.js. This icon mapping would precisely specify the icons that skin provides.

Then, at some later stage, we download a new skin and tell the editor to use it instead. This new skin provides 10 icons only. But still the hardcoded CKEDITOR.skin.icons says that the skin has 15 icons, which the editor will try to use. At that point things will be broken.

That's why we needed to find a way for the skin to tell which icons it provides, without forcing the editor to download additional files. The only option we had for it is by using the skin CSS file.

comment:11 in reply to:  8 ; Changed 6 years ago by Garry Yao

Replying to garry.yao:

For the plugins icons (sprites) in released copy though, ckbuilder need to be fixed, it currently append a chunk of code to the core file (ckeditor.js) that manipulates the icon definitions, but unfortunately RTL version is missing, so I've opened t/9467 on ckbuilder repo to start making this change.

@Wiktor, can u check this change and eventually make a update to the builder for that?

Wiktor has pointed out ckbuilder currently use a single strip for all RTL and LTR icons, so no change is needed at the moment, to make this work, fortunately.

comment:12 in reply to:  11 Changed 6 years ago by Piotrek Koszuliński

Replying to garry.yao:

Replying to garry.yao:

For the plugins icons (sprites) in released copy though, ckbuilder need to be fixed, it currently append a chunk of code to the core file (ckeditor.js) that manipulates the icon definitions, but unfortunately RTL version is missing, so I've opened t/9467 on ckbuilder repo to start making this change.

@Wiktor, can u check this change and eventually make a update to the builder for that?

Wiktor has pointed out ckbuilder currently use a single strip for all RTL and LTR icons, so no change is needed at the moment, to make this work, fortunately.

Is it? I don't see a reason for that. In v3 we were replacing entire strip for all buttons at once, but in v4 we've got separate selectors, so we can have everything in one strip. Can't we?

comment:13 Changed 6 years ago by Wiktor Walc

just a note that the t/9467 branch + CKBuilder 1.5 (an update in build.sh is needed) should work in source and release version, at least it seems to work after a quick check.

To make the testing process easier, I uploaded also a test version 1.5.1, that contains slightly modified CSS rules, suggested by Garry.

Last edited 6 years ago by Wiktor Walc (previous) (diff)

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

Status: reviewreview_failed

I confirm that t/9467 worked, but after last change to builder I see in RTL editor kama's icons :|.

I also pushed small code change.

comment:15 Changed 6 years ago by Garry Yao

Status: review_failedreview

I've pushed commits to t/9467 on dev and t/9467b on builder for review.

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

Status: reviewreview_passed

Ok, looks fine. I've found one issue with missing textfield-rtl icon in release version, but this deserves new ticket (it's not a regression).

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

Status: review_passedreview_failed

I changed my mind after reviewing builder changes. In lines src/lib/image.js:147-161 may be a problem with files list. If ltr version of icon will be first (and there's no guarantee about files order) iconsHasRtl flag will be changed too late.

I guess that we just have to use sort() on files array to ensure correct order.

Last edited 6 years ago by Piotrek Koszuliński (previous) (diff)

comment:18 Changed 6 years ago by Garry Yao

Status: review_failedreview

The icons list is created from an iteration, theoretically for...in loop in Rhino 1.7 sorts the keys alphabetically (sortedHashMap), so icons from the same button are always adjacent, though I've committed a change to ensure that.

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

Owner: changed from Garry Yao to Piotrek Koszuliński

I pushed corrected icons sorting - that has to be done by paths (ico-rtl.png,ico.png), not by names (ico,ico-rtl).

And I confirmed that indeed order is random - at some point some RTL icons were before their LTR clones, but the rest was ok.

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

Resolution: fixed
Status: reviewclosed

I masterised both branches - on dev and builder.

https://github.com/ckeditor/ckeditor-dev/commit/1aab3a2

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