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

Enable two-step caret movement for code attribute. #7356

Closed
wants to merge 15 commits into from

Conversation

tomalec
Copy link
Contributor

@tomalec tomalec commented Jun 1, 2020

Suggested merge commit message (convention)

Other (basic-styles): Enable typing around for code element. Closes #6722.


Additional information

The change is mostly copy-pasted from LinkEditing

I'm not sure whether such a tiny detail is worth mentioning in high-level docs at https://ckeditor.com/docs/ckeditor5/latest/features/basic-styles.html#available-text-styles.

Also, I wasn't sure whether highlighting should go into the scope of this issue. I made a draft at i/6722-code-caret-highlight. I think If it should be implemented here, I'd make a shared util/mixin so the code could be shared between Link and Code.

@tomalec tomalec force-pushed the i/6722-code-caret branch from 8d73e23 to aa5c686 Compare June 1, 2020 15:47
@tomalec tomalec marked this pull request as ready for review June 1, 2020 16:42
@tomalec
Copy link
Contributor Author

tomalec commented Jun 2, 2020

We (@oleq, @niegowski and me) agreed today on F2F meeting, that visual feedback is necessary.

I'll continue as started on i/6722-code-caret-highlight branch, move highlighting code to ..engine/src/utils/bindtwostepcarettoattribute.js, make Link use it as well, and mention it in the docs.

@tomalec
Copy link
Contributor Author

tomalec commented Jun 7, 2020

I've made link's highlight code slightly more re-usable,
used it for code as well, mentioned two-step movement in code feature docs.

The highlight effect is not visible in docs, due to a selector with higher specificity at https://github.com/cksource/umberto/blob/b8c9196cd2e653f2cf820be3291279a53bf83d9c/themes/umberto/src/css/_formatted.scss#L100

If the changes in this PR get approved, I'll create a followup PR there.

@oleq oleq self-requested a review June 8, 2020 09:50
@oleq oleq self-assigned this Jun 8, 2020
Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't go through the code but I found some major issues from the user standpoint. The bindTwoStepCaretToAttribute helper was created without considering a "multiple attributes scenario" (we used it for link only) so there are bugs and glitches and probably some bad UX decisions we must address first. A linked code is a very common use-case (see: our project documentation, GitHub Writer, etc.) so the integration must be flawless.

As for the UX, I suppose the caret movement should work for the outermost attribute first (linkHref) and then for the inner attribute (code) so, for instance, it is possible to type a link which is no longer a code, but it is also possible to continue typing a linked code. It is also critical the editor announces the caret state correctly (highlighting) so the user knows what happens if they start to type.

The bindTwoStepCaretToAttribute needs more tests to cover all multiple attribute scenarios and then the code must be fixed to get them green.

Some of the glitches I found (tested in docs)

You'd expect that the second Arrow left does something (activates the code or a link, or both?) but it doesn't. If you do 2 xArrow left and then you type, you still get a plain text.


This scenario is also unstable (why is the space neither a link nor a code?):

this is what the model looks like after the scenario (note that the space not only lacks attributes but also gets inserted twice):


Or check out this one (one of my favorites):

Probably there are more glitches like this. Thorough automated tests should flush them out.

@tomalec
Copy link
Contributor Author

tomalec commented Jun 9, 2020

Thanks, I'll cover those cases and add tests. I was not able to reproduce the first case, but maybe I'll get to it when investigating and solving others. Otherwise, I'll bother you a little, to help me with that.

@oleq
Copy link
Member

oleq commented Jun 9, 2020

The last one looks like #1108.

@tomalec
Copy link
Contributor Author

tomalec commented Jun 12, 2020

I was not able to reproduce the highlight issue - I have added a test to check against that.

I was trying to address "the same character" issue, but with no luck. I've added the test to make sure it's not the result of 2SCM. Also, the issue is reproducible after commenting out the 2SCM and highlight code.

I'll spend a few hours more trying to find a solution. In case I would not find any by Monday, I think this PR could be reviewed again, and follow it up at #1108.

@oleq
Copy link
Member

oleq commented Jun 15, 2020

I was not able to reproduce the highlight issue - I have added a test to check against that.

Take a look at this screencast, the left arrow is pressed thrice. 

Actual

  1. The first key press moves the selection and overrides the gravity. If you start typing, a plain text is entered (✅ OK)
  2. The second one does nothing, it does not change the selection/selection attributes/gravity. If you start typing, a plain text is entered again (❌ NOK)
  3. The third one restores the gravity which gives the selection both code and linkHref attributes. If you start typing, a linked code is extended (✅ OK)

Expected

  1. The first key press moves the selection and overrides the gravity.
  2. The second key press allows you to continue typing in a link (but not a code)
  3. The third key press allows you to type a linked code.

@tomalec
Copy link
Contributor Author

tomalec commented Jun 15, 2020

Ok, I'll get into this, looks similar to #946

tomalec added a commit that referenced this pull request Jul 8, 2020
@tomalec
Copy link
Contributor Author

tomalec commented Jul 8, 2020

Closing in favor of #7577

@tomalec tomalec closed this Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow distinguishing between "cursor inside/outside inline code" (same as for links)
2 participants