Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Behaviour of sub/sup features #5556

Open
Reinmar opened this issue Aug 31, 2018 · 15 comments
Open

Behaviour of sub/sup features #5556

Reinmar opened this issue Aug 31, 2018 · 15 comments
Labels
package:basic-styles status:discussion type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Reinmar
Copy link
Member

Reinmar commented Aug 31, 2018

The ckeditor/ckeditor5-basic-styles#74 PR by @idleb introduces basic subscript and superscript support. However, since it's implemented in the same way than bold/italic/etc it comes with their semantics:

  1. The order in which you apply styles does not count – they are always output in the same order (i > strong). With sub/sup you may want to create sup > sub or sub > sup. It simply depends on the semantics of the text and cannot be hardcoded by the editor.
  2. You cannot apply sup or sub twice to the same text. It is possible (although very unlikely) that you may want to write sub > sub or sup > sub > sup. That won't be doable with how this feature is implemented.

In CKEditor 4 the (2) is not possible but (1), the order, is controllable. In CKEditor 5 both are not doable now and while we can ignore (2) I'm afraid about (1). The biggest problem here is that it won't be that small change. Also, regarding (2), I remember that it was requested by the users of CKEditor 4 (IDK how many though).

But, before we'll sink into this, are these limitations important to you? Please 👍 this ticket and leave comments which parts are crucial for you.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 3, 2018

Regarding the order (1) – the current order how e.g. bold and italic are output base, I think, on the alphabetical order (so to be consistent). There's also the concept of priorities of AttributeElements (and it's e.g. used by the link feature to output <a> as the outermost element), but in this case, both elements have the same priority so I think we use alphabetical sort.

It came to my mind though, that we could use current date as a value of sub/sup model attributes and use it later, with conjuction with some base priority to change the order of sub and sup in the view. For example, let's say the base priority is 10. So, sub created at new Date().getTime() has priority:

10 + new Date().getTime() / 10000000000000 ~= 10.1535966907083

A couple of seconds later:

10.1535966958953

Etc. With every second it gets a higher priority so sub/sups created later get inside (or outside, however it should work) of sub/sups created earlier.

I wonder if this may work :D It seems to be a hack but perhaps once we analyse this it may turn out that it's indeed what we need.

PS. The only problem is if you have two different sub values next to each other. Do we want to merge them? If yes, then the above algorithm needs to be reviewed.

@scofalik
Copy link
Contributor

scofalik commented Sep 3, 2018

It won't work for removing (unwrapping) such AttributeElement without some additional work. Because of different priorities.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 3, 2018

Why? The priority will be the same – it'll be taken from the sub/sup model attr. I want to store the date in the model.

@scofalik
Copy link
Contributor

scofalik commented Sep 3, 2018

Oh sorry, missed that part.

@scofalik
Copy link
Contributor

scofalik commented Sep 3, 2018

As far as creating sub in sub and sup in sup: how do you see UX working for that? When should the button be "clicked" so you are able to remove sub/sup instead of creating a new one? When exactly the whole sub/sup element is selected?

@pjasiun
Copy link

pjasiun commented Sep 3, 2018

I think that to implement it, we need to add some priority information, most probably also to the model, and then take it into consideration during the conversion.

I think, instead of using the timestamp we could check what priorities other elements has and use an incremented value.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 4, 2018

Answering @scofalik. Check out CKEditor 4. When you're in:

<p><sub>Foo[Bar]</sub></p>

And you:

  • click the Sub button you remove that style,
  • click the Sup button you get: <p><sub>Foo<sup>[Bar]</sup></sub></p>

So, the order of styles is that the style which is applied later ends inside the style applied earlier.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 4, 2018

Answering @pjasiun.

I think, instead of using the timestamp we could check what priorities other elements has and use an incremented value.

How's that different from using a timestamp? You know the base priority of Sub/Sup. So when you want to decrement/increment, you just need to add whatever to it.

@pjasiun
Copy link

pjasiun commented Sep 4, 2018

The difference is not big. I just think it is nicer to use 11 than 10.1535966958953. And it should give us smaller messages in collaboration, but that basically it.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 4, 2018

Hard to tell whether int priorities are going to work in a longer term. Depending on how we'd use them when applying and removing the styles a couple of time we might quickly end up with priorites like 20 and that would affect how this feature is rendered next to some other inline styling. That's why I'd try to operate within a small range like 10-11. But we do not have to use timestamps for that naturally. I just thought it may a trick worth considering.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-basic-styles Oct 9, 2019
@mlewand mlewand added this to the unknown milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:improvement This issue reports a possible enhancement of an existing feature. package:basic-styles labels Oct 9, 2019
@pomek pomek removed this from the unknown milestone Feb 21, 2022
@quicksketch
Copy link

I think <sub> and <sup> tags should be made mutually-exclusive. If you click the Subscript button on Superscripted text, it should switch from one tag to the other. While it makes a lot of sense that you'd want to both bold and italicize (or underline), both super and subscripting text doesn't make sense.

@wimleers
Copy link

wimleers commented Oct 3, 2023

Agreed with @quicksketch, and that'd make this far simpler to implement too! Easy for both end users and the maintainers 👍

@wimleers
Copy link

wimleers commented Oct 3, 2023

Related bug: #15117

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@quicksketch
Copy link

Still interested in this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:basic-styles status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

8 participants