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 spellcheck for EasyMDE, use contenteditable mode #19776

Merged
merged 6 commits into from
Jun 28, 2022

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented May 21, 2022

As the title.

Fix #18532

And: CodeMirror docs indicate that "contenteditable" be the future default on Desktop as well, so it's good to use contenteditable from now on.

Tested browsers:

image

@wxiaoguang wxiaoguang added this to the 1.17.0 milestone May 21, 2022
@wxiaoguang wxiaoguang marked this pull request as draft May 21, 2022 14:57
@wxiaoguang wxiaoguang changed the title Enable spellcheck for EasyMDE (WIP) Enable spellcheck for EasyMDE May 21, 2022
@wxiaoguang wxiaoguang added the pr/wip This PR is not ready for review label May 21, 2022
@wxiaoguang wxiaoguang removed this from the 1.17.0 milestone May 24, 2022
@silverwind
Copy link
Member

Seems to work again on Firefox Mac when triggering spellcheck via context menu.

image

contenteditable generally makes me wary, but CodeMirror docs indicate that it'll be the future default on Desktop as well, so I trust their implementation to be stable enough.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 24, 2022
@jolheiser
Copy link
Member

I believe this was disabled for various reasons related to SimpleMDE.
#9619
#10729

Perhaps the native one wasn't available back then? I don't recall.
Just posting here for context, I'm generally in favor of enabling it. 🙂

@tyroneyeh
Copy link
Contributor

Turning on the upload function of this image will not work

@wxiaoguang
Copy link
Contributor Author

Turning on the upload function of this image will not work

Yup, as mentions in the Problems / TODO section:

the image paste and other features need to be rewritten.

@tyroneyeh
Copy link
Contributor

Sorry i didn't notice, i tried changing it and it seems to work,
This attache patch is base on release/v1.16 branch for reference

ImagePaste.zip

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Jun 23, 2022

Awesome 👍 I applied your patch.

I will do more tests with it later and confirm some details (eg: $(document).on('paste', '.CodeMirror', ... seems to be executed more than one time). Feel free to suggest more 😊


Hmm ... maybe initCompImagePaste and initEasyMDEImagePaste should be merged together. 🤔️

@tyroneyeh
Copy link
Contributor

tyroneyeh commented Jun 23, 2022

Sorry for my patch but didn't work in the edit comments :(

@tyroneyeh
Copy link
Contributor

Sorry for my patch but didn't work in the edit comments :(

Edit problem is my modify indexer.go issue, please skip it

@wxiaoguang
Copy link
Contributor Author

Sorry for my patch but didn't work in the edit comments :(

Never mind, I fixed them all.

@wxiaoguang wxiaoguang marked this pull request as ready for review June 23, 2022 12:33
@wxiaoguang wxiaoguang removed the pr/wip This PR is not ready for review label Jun 23, 2022
@wxiaoguang wxiaoguang changed the title (WIP) Enable spellcheck for EasyMDE Enable spellcheck for EasyMDE Jun 23, 2022
@wxiaoguang wxiaoguang added this to the 1.18.0 milestone Jun 23, 2022
@tyroneyeh

This comment was marked as off-topic.

@tyroneyeh
Copy link
Contributor

Safari is work

image

@wxiaoguang

This comment was marked as off-topic.

@wxiaoguang
Copy link
Contributor Author

Safari is work

Thank you for confirming. I think this PR is ready for review.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 27, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 28, 2022
@jolheiser
Copy link
Member

🎺 🤖

@wxiaoguang
Copy link
Contributor Author

(FYI: the last force-push brings no change, the last commit is still 7c44a3a)

@wxiaoguang wxiaoguang merged commit 76910f2 into go-gitea:main Jun 28, 2022
@wxiaoguang wxiaoguang deleted the enable-spellcheck-easymde branch June 28, 2022 17:53
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 29, 2022
* upstream/main:
  [skip ci] Updated translations via Crowdin
  Enable spellcheck for EasyMDE, use contenteditable mode (go-gitea#19776)
@wxiaoguang
Copy link
Contributor Author

This feature has been deployed on try.gitea.io

Feel free to test it and report bugs.

If there is something broken seriously, we should either fix it or revert this feature. Hopefully everything goes well.

@tyroneyeh
Copy link
Contributor

There is a chance of missing characters in the Linux ibus input method framework

@wxiaoguang
Copy link
Contributor Author

Besides Linux IME, the mobile IME may also be affected: Contenteditable on Android is the Absolute Worst

If the EasyMDE/CodeMirror still has difficulty to work correctly in some browsers, then it's worth to add a default editor mode option (before the CodeMirror can be fixed or replaced totally), use a simple textarea editor instead of the CodeMirror.

@silverwind
Copy link
Member

silverwind commented Jul 17, 2022

If the UI would remember the switch to textarea per-user, it would be a big improvement. And yeah, there should be a server setting for the default.

@wxiaoguang
Copy link
Contributor Author

Good idea, maybe remember it in LocalStorage is enough, then no need to write backend code.

@silverwind
Copy link
Member

silverwind commented Jul 17, 2022

We need some simple schema-less key/value JSON ui settings in the db for ui settings like this. localStorage is a start but it should ideally sync across browers.

@delvh
Copy link
Member

delvh commented Jul 17, 2022

models/user/setting.go?

@silverwind
Copy link
Member

Ah yes that seems suitable, I totally forgot we landed that :)

@tyroneyeh
Copy link
Contributor

If the nativeSpellcheck default is false, an icon is added to the toolbar as a switch, do you know if there is still a problem?
{ name: 'spell-check', action(e) { const cm = e.codemirror.getTextArea().parentElement.querySelector('.CodeMirror-code'); cm.spellcheck = cm.spellcheck ? false : true; }, className: 'fa fa-check', title: 'Spell Check' }

image

@wxiaoguang
Copy link
Contributor Author

The usability problem is not related to spell-check but the editor itself (inputStyle).

@tyroneyeh
Copy link
Contributor

Maybe let the user check spelling by themselves, and set contenteditable to false if they continue to type

easyMDE_spellcheck.zip

@techknowlogick
Copy link
Member

locking this, as closed PRs are the best place to ensure conversation is lost/not visible to everyone. please open a new ticket to discuss.

@go-gitea go-gitea locked and limited conversation to collaborators Jul 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

web interface - spellchecker of the browser not available
9 participants