web-ext-translator icon indicating copy to clipboard operation
web-ext-translator copied to clipboard

Make preview feature opt-in

Open rugk opened this issue 6 years ago • 8 comments

I don't get why it includes a markdown preview. Usually you only have text – and when you have more, it is very likely you include HTML text, but not Markdown?

So why?

rugk avatar Jul 11 '18 17:07 rugk

Markdown is a lot easier for non-developers to work with than HTML. It's also less dangerous. If you're not careful, someone might sneak in JavaScript into that HTML code.

I was thinking about allow making the preview flexible enough to display different content types, but that would have to be configured by the extension authors by supplying a config file, which describes the preview type.

Lusito avatar Jul 11 '18 20:07 Lusito

If you're not careful, someone might sneak in JavaScript into that HTML code.

Of course, but you should trust & review your JSON files. Also even Markdown needs to be passed through a DOM sanitisation tool or so as it can also include HTML.

Also you can hardly assume anyone would write Markdown in these JSON files, as that is not a documented feature of these files.

but that would have to be configured by the extension authors by supplying a config file

Why not just use comments on the JSON for that?

rugk avatar Jul 12 '18 12:07 rugk

Also even Markdown needs to be passed through a DOM sanitisation tool or so as it can also include HTML.

I use markdown-it for both the editor and my add-ons and it already handles these issues: https://github.com/markdown-it/markdown-it/blob/master/docs/security.md

Of course you should check these, but humans make flawed, so it can't hurt to add some security layers.

Why not just use comments on the JSON for that?

Either that or a real data entry in the messages.json would of course be possible. I haven't worked this out yet.

Lusito avatar Jul 12 '18 16:07 Lusito

but humans make flawed, so it can't hurt to add some security layers.

Well… devs should check the stuff they have in their repo. If there is rogue stuff in their rrepo and they don't see that, this can be worse than just in some translation files. But getting off-topic. Yes, of course it's good to have this "layer of defense". Basically, the lib looks great and by being more secure, it's also more strict.

However, it's still that you only use that. My point is: Having a Markdown preview there kinda suggests add-on devs that this is the way to go, how they should/can do it. So this may be misleading advise. (And I know, not a deliberate advise, but it is implied there.)

Either that or a real data entry in the messages.json would of course be possible. I haven't worked this out yet.

So I'd argue for just disabling the preview unless you find such tags, i.e. disable it for now and once it is implemented the preview may be shown, and it may only be shown for these entries, where it is appropriate (i.e. "annotated"). :smile:

rugk avatar Jul 12 '18 19:07 rugk

Not sure why this is such a problem, you can press the eye button in the toolbar and it goes away.

Of course, it can be implemented, that the preview will only be shown when a message has been marked as markdown, but I won't disable it until then. I write my help pages and my change-logs as markdown and need it as a major feature of the editor

Aside, I'm not sure I want to disable it for non-markdown entries:

  • A preview might still be of interest to see how they look with placeholders replaced.
  • Enabling/disabling the sidebar whenever a markdown or non-markdown editor has been selected will result in annoying UI layout rearrangements.

I guess I could do one or more of the following:

  • Create a setting for the extension developers to hide it on a per-extension case rather than on a per-message case.
  • Set the initial visibility of the sidebar to disabled for extensions that do not have a message marked as markdown.
  • Remember the toggled state in the browser storage for your next visit.

Lusito avatar Jul 12 '18 20:07 Lusito

How would you solve these complex views like help pages if not by using markdown or similar? I don't see why markdown is a big no-no for extension developers.

Lusito avatar Jul 12 '18 20:07 Lusito

How would you solve these complex views like help pages if not by using markdown or similar?

HTML.

Or, generally in your case you may split it into smaller parts. As if one typo is fixed in this huge translation it is "fuzzy" and the whole thing has to be translated.

Or, e.g. for add-on pages or so, which are not really used in the add-on, I just use text/html/md files in an extra assets dir. You could somehow do the same by checking which language you need to load and load the file that corresponds to that in an add-on.

I don't see why markdown is a big no-no for extension developers.

That's not my point. My point is just: It's just not the only way. and...

Not sure why this is such a problem, you can press the eye button in the toolbar and it goes away.

Yes, but showing the preview for "not MD strings" is unneccessary. That's why I'd prefer markers to opt-in to "This is a markdown string.".

A preview might still be of interest to see how they look with placeholders replaced.

So that's another kind of preview. I like the idea, but you don't need a sidebar for this. Here you could e.g. show it on mouse over.

will result in annoying UI layout rearrangements.

Hmm... fair point. That could be a good reason.

I guess I could do one or more of the following:

Yeah, these are all good ideas. Altghough I'd prefer that developers "opt-in" to a preview feature, because I guess not many will need it. If you only have small strings and no or only few placeholders, the preview is not of much use.

rugk avatar Jul 13 '18 08:07 rugk

If you use HTML, your translators need to understand HTML as well. It's a lot easier to teach people correct Markdown than correct HTML.

Anyway, there will be options for this, but I will try and collect some more input from other developers to make sure I get this right.

Lusito avatar Jul 14 '18 19:07 Lusito