DSpace / dspace-angular

DSpace User Interface built on Angular.io
https://wiki.lyrasis.org/display/DSDOC8x/
BSD 3-Clause "New" or "Revised" License
135 stars 434 forks source link

Enable / disable textarea spellchecking via config #1798

Closed mark-cooper closed 1 year ago

mark-cooper commented 2 years ago

Is your feature request related to a problem? Please describe.

The v7 ui app is generating textarea form elements with explicit spellcheck=false. This disables browser based spellchecking functionality (which I'm informed worked fine in v6), without additional clicks to make it happen.

Describe the solution you'd like

Ideally there would be at least a config option for generating textarea form elements with spellcheck=true so browser spellchecking can work at least with parity to v6.

Describe solutions you've considered

This branch makes it so that browser spellchecking can be enabled / disabled via the config file by passing through the config value to DynamicTextAreaModel / DsDynamicTextAreaModelConfig:

https://github.com/DSpace/dspace-angular/compare/main...mark-cooper:dspace-angular:textarea-spellcheck

Happy to make a PR if this looks generally useful.

Before:

before

After (form.spellCheck: true):

after

mwoodiupui commented 2 years ago

I was going to say that spellcheck-or-not is a personal issue and should be a user profile option rather than a global one, but then realized that browsers probably make this personally configurable, and on checking found that at least Firefox does. So maybe the best thing to do would be to just remove the parameter from these forms and leave it to browser settings.

mark-cooper commented 2 years ago

"So maybe the best thing to do would be to just ... leave it to browser settings"

I think spellcheck=true effectively does this. If the browser settings are set to not spellcheck (for example, Firefox "Check your spelling as you type" disabled), then the attribute value (spellcheck=true) is ignored. But if you do want spellcheck ("Check your spelling as you type" enabled ) you're good.

With the current situation you can't "passively" opt-in to spellcheck. You have to force it (Right click -> Check Spelling [Firefox]), because of the generated spellcheck=false. The feedback I've received is that this isn't desirable, and different from v6.

Not generating the attribute at all would be more or less equivalent as spellcheck=true, although leaving it a bit more to browser "choice" on whether spellchecking is applied in the absence of a spellcheck attribute.

alawvt commented 2 years ago

We rely heavily on the default spell checking to catch metadata errors. We favor the current behavior in DSpace 6.

tdonohue commented 2 years ago

Just re-discovering this ticket, and I agree that it seems reasonable to attempt to replicate DSpace 6 behavior with regards to spellcheck. So, I'd be OK with an option to make this configurable, or remove spellcheck=false altogether (I haven't had a chance to check yet which option better aligns with DSpace 6)

UPDATE: Realized that @mark-cooper linked to his code updates in the description. Those code updates look reasonable / non-intrusive to me, so I'd be fine with passing those changes on to 7.5 reviewers/testers if you can create a PR, @mark-cooper !

mark-cooper commented 2 years ago

@tdonohue I think I'll have time to do it over the next couple of weeks 👍

mark-cooper commented 1 year ago

Oof, I got well behind on this one.

@tdonohue I rebased the original branch and retested it:

https://github.com/DSpace/dspace-angular/compare/main...mark-cooper:dspace-angular:textarea-spellcheck

Checks out.

Alternatively it's much simpler (less boilerplate) if we can assume spellcheck is generally desirable by default. It can always be disabled by the user in browser:

https://github.com/DSpace/dspace-angular/compare/main...mark-cooper:dspace-angular:textarea-spellcheck-enabled

So perhaps the simpler (in line with v6) approach is better?

I don't believe it's currently possible to not generate a spellcheck attribute:

https://github.com/udos86/ng-dynamic-forms/blob/master/projects/ng-dynamic-forms/core/src/lib/model/input/dynamic-input.model.spec.ts#L89-L91

But I haven't really pushed against it.

tdonohue commented 1 year ago

@mark-cooper : Thanks for getting back to this. Giving the two options you propose, I have a slight preference for making this configurable (the first branch you linked) & defaulting it to true.

While I understand that hardcoding it to true is easier & doesn't force the browser to do a spellcheck, I think there's a chance some DSpace sites may want it to be false because of the "Security and privacy concerns" listed in the docs for this attribute: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/spellcheck

So, if you could create a PR based on your first branch (https://github.com/DSpace/dspace-angular/compare/main...mark-cooper:dspace-angular:textarea-spellcheck), I think that'd be wonderful!