Opened 6 years ago

Closed 6 years ago

#10856 closed Bug (fixed)

It is not possible to remove lang attribute using language plugin

Reported by: Piotr Jasiun Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.3
Component: General Version: 4.3 Beta
Keywords: Cc:

Description (last modified by Piotr Jasiun)

A cat add eg. lang="es" with language button and I can change the language, but it is not possible to remove it without source editing.

Change History (15)

comment:1 Changed 6 years ago by Piotr Jasiun

Description: modified (diff)

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

Status: newconfirmed

I pushed a solution to t/10856. It needs tests now.

comment:3 Changed 6 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedreview

Your code handled job of removing element.

I've added few missing UX funtionallities:

  • added tests for removing
  • indication on language plugin button if any language is currently applied
  • indication which language is currently active
  • added dedicated button for removing after clicking on languages button

Pushed to t/10856 (dev and tests)

Last edited 6 years ago by Marek Lewandowski (previous) (diff)

comment:4 Changed 6 years ago by Marek Lewandowski

Additionally Olek should take a look at visual changes made within this ticket (that's highlight of active dropdown entry, in this case highlighted language).

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

Status: reviewreview_failed

Force pushed t/10856 branches, rebased on major.

  • Please see http://dev.ckeditor.com/ticket/10862#comment:4 - there are some code style issues in t/10856 too.
  • I think that getCurrentPathIndicatior may be solved easily with elementPath#contains.
  • {CKEDITOR.dom.element|null} this is incorrect - we use / to separate types.
  • items['lang-remove'] this is incorrect - there are always spaces inside nonempty [].
  • The state of button should be refreshed on selectionChange, not on elementsPathUpdate. The latter one is a non-core event fired on selectionChange.
  • getCurrentLangIndicator is documented as a public method but it's not associated with any class. If it has to be public (has it?) it should be defined in CKEDITOR.plugins.language. Otherwise, it should be hidden.
  • In getCurrentPathIndicatior editor.elementPath() should not be called twice.
  • Olek should indeed check the styling.
  • I'm not sure whether we need the last option of languages drop down.

comment:6 in reply to:  5 ; Changed 6 years ago by Frederico Caldeira Knabben

Replying to Reinmar:

  • I'm not sure whether we need the last option of languages drop down.

Yes, better to have it. One would not understand straight that it's enough to click the active language to remove it.

We should try to have a separator before it though.

comment:7 in reply to:  6 Changed 6 years ago by Olek Nowodziński

Replying to fredck:

Replying to Reinmar:

  • I'm not sure whether we need the last option of languages drop down.

Yes, better to have it. One would not understand straight that it's enough to click the active language to remove it.

We should try to have a separator before it though.

I don't like it either. But if you insist, it certainly it cannot be "Remove formatting" but something like "Reset to default". Language isn't formatting.

comment:8 Changed 6 years ago by Olek Nowodziński

A related issue is #10889.


Anyway, I found a selection-related issue there:

  1. Select
    [ was the spaceflight that landed the first humans]
    
  2. Set French.
  3. Select
    [spaceflight that landed]
    
  4. Set Spanish.
  5. Put the caret
    spaceflight ^that landed
    
  6. Click "Remove formatting"
  7. Caret landed far away
    was the spaceflight that landed the first humans^
    

A funny remark: I'm unable to reproduce it with Arabic (dir="rtl").

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

Pushed two commits to DEV to unify CSS:

  • Re-used styles for active elements in combo for active menu items.
  • Minor CSS tweaks.

comment:10 Changed 6 years ago by Marek Lewandowski

Status: review_failedreview

There was major extra fix, all item members have been prefixed with "language_", because created index is used in CKEDITOR.instances.editor1._.menuItems storage object. Without it, we may end up with members like CKEDITOR.instances.editor1._.menuItems.en which could be confusing, and vulnerable for conflicts.

Pushed to t/10856 dev and t/10856 tests.

comment:11 Changed 6 years ago by Frederico Caldeira Knabben

I've just deleted origin/t/10856 :P

comment:12 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

I think that the button state should be simply linked to the command state. So, state checks should be done by the command itself, instead of doing it through the instanceReady event. Let's talk a bit about this and see if this improvement can be done.

comment:13 Changed 6 years ago by Marek Lewandowski

Status: review_failedreview

Some fixes pushed to t/10856 (dev only). Note there was notable change to menubutton plugin (it will behave same as panelbutton and richcombo when clicked multiple times)

comment:14 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

I've made some minor commits on the branch and updated both dev and tests to the current major.

comment:15 Changed 6 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

Fixed on major with ​git:2c90b876 on dev and 7938641 on tests.

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