Opened 11 years ago

Closed 11 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 11 years ago by Piotr Jasiun

Description: modified (diff)

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

Status: newconfirmed

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

comment:3 Changed 11 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 11 years ago by Marek Lewandowski (previous) (diff)

comment:4 Changed 11 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 11 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 11 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 11 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 11 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 11 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 11 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 11 years ago by Frederico Caldeira Knabben

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

comment:12 Changed 11 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 11 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 11 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 11 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 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy