Warn users if the visual editor will modify their code, and offer to switch to the raw editor.

[davestgermain]

WIP

There aren't any tests yet, but I wanted to see what people thought about this approach. Currently, the confirmation is a javascript confirm dialog. Should it be a modal -- if so, where can I find that? What should the messaging be? Now, it's just a feeble: Visual editing may cause data loss. Click cancel for raw editor.

The problem I'm running into now is that tinymce is adding a \ufffd character surrounded by a hidden span. I don't know why, but it seems related to the valid_elements setting...

@cahrens @nasthagiri @andy-armstrong

[cahrens]

@davestgermain Can you create a sandbox for people to try it out?

[cahrens]

@nasthagiri Did you think we would explicitly list all the valid_elements (and extended_valid_elements)? I thought the goal was to rely on TinyMCE's default list of valid_elements, and only add ones that we know we need.

[nasthagiri]

@cahrens Yes, that's correct - that was the original plan. TinyMCE had recommended that we set the valid_children attribute for +body[style], per this Fiddle (http://fiddle.tinymce.com/0eeaab/2), but not the valid_elements attribute.

But this is why there's a small discovery part to this story. If we find there's no downside to Dave's approach here..

[davestgermain]

@cahrens @nasthagiri I've set up a sandbox here: http://studio.davestgermain.m.sandbox.edx.org

[cahrens]

I question the approach of explicitly listing tags. @davestgermain, do you know why I am getting the "Raw Editor" warning when I try to edit our Announcement template? I don't see anything "dangerous" in that.

I also think we need a better dialog. Pressing "Cancel" should be a no-op, but currently it is changing you to the Raw Editor.

UX team is off-site, but I will encourage Lou and @frrrances to take a look at this PR tomorrow so we can get this moving again.

[frrrances]

@davestgermain: The sandbox doesn't seem to be working... can you set it up again and I'll take a look today?

[lou-wang]

@davestgermain Same problem with the sandbox not responding.

[davestgermain]

@frrrances @lou-wang the sandbox is running again

[lou-wang]

A few thoughts:

1. Can we display the "Hey you might lose data" modal after the course author clicks Edit on the HTML but before the actual visual editor renders?
2. The "Hey you might lose data" should be a system modal. I think there should be three options, something like this: "This component contains HTML tags that are not supported in Visual Editor. To ensure you don't lose any data, you should edit this component using the Raw HTML editor. [Use Raw Editor] [Use Visual Editor] [Cancel] Thoughts @frrrances @mhoeber?
3. If possible, it'd be nice to issue a "Hey you might lose data"-type warning when an author tries to convert a Raw editor into a Visual editor.

[mhoeber]

I agree that Cancel shouldn't cause the switch to the Raw editor. But assuming it's labeled something like "Raw Editor", I find it confusing that it's doesn't really change the setting -- the Raw editor opens, but Visual is still selected in the settings, and you keep repeating the dialog in repeated sessions.

@lou-wang I'm not sure of the use case that would really lead to #1 above. We should have a policy not to have templates that default to the visual editor that cause this warning -- if we're providing the template, then it should not have any risk. That would mean the dialog would only appear after a user modified the content, and if they were modifying in the visual editor then it wouldn't be an issue. If that's the case, the dialog would just make sense when you switched the Editor from Raw to Visual (#3 above). And in that case, it would be friendlier to be very explicit in the Settings tab inline help instead of a dialog. I can update the dialog text to strengthen if we agree.

[lou-wang]

@mhoeber Case #1 occurs when a course team authored content with complex HTML before we rolled out our recent changes. It is the most important case we have to cover, because it is currently resulting in data loss.

[mhoeber]

@lou-wang thanks, wasn't thinking of that. Probably not but is there a way to have this dialog either based on creation date, or appear just once per component?

[lou-wang]

@mhoeber Well, if there's no unsupported tags, then the error will never appear, so I don't think we need a date-based check. And once this error appears, the user will have to make a choice - either convert to RAW, or let the Visual editor blow away unsupported tags. Once that choice is made, they'll never see the error again. So my inclination is that it's fine without any additional check.

[mhoeber]

@lou-wang that would be great. when I used, it was appearing every time for the announcement template.

[davestgermain]

@lou-wang I looked into showing the warning before the editor modal appears, but I can't find any good hook to insert the check, without adding an ugly hack just for HTML components. Does anybody have any ideas?

Also, I agree that the dialog should be a modal dialog. Is there a generic modal dialog somewhere in studio? @frrrances?

What do I have to do to move this forward, as I'm a bit confused?

[cahrens]

I am still not a fan of the explicit listing of tags (as opposed to @nasthagiri's proposal).

@davestgermain , did you figure out why you get a warning when editing the Announcement template? I don't see anything "dangerous" in that.

I'm concerned that we may be directing people to the raw editor too aggressively.

[davestgermain]

@cahrens I thought you told me to limit the HTML to the strict subset allowed by the GUI. The TinyMCE default would allow much more and thus would rarely trigger a warning (but would still silently mangle the HTML).

The announcement triggers a warning because there's a section tag.

[cahrens]

@davestgermain Where did you get the "strict subset allowed by the GUI"? I thought we were using the TinyMCE defaults minus known problematic tags. This is all we had specified in STUD-1576:
"The whitelist is the TinyMCE supported tags list minus known problem tags including < script >"

Why should we disallow section tags?

[davestgermain]

@cahrens Well at this point I don't remember who told me that. I thought the idea was that the only HTML allowed in the visual editor should be HTML that could be created by the visual editor -- everything else should be kicked to the raw editor.

The issue is that TinyMCE will silently mangle the HTML, which apparently we don't want. Excluding only "known problematic tags" doesn't do enough to prevent this re-writing.

[sarina]

@davestgermain are you still working on this PR? If so can you help get it through the review pipeline?

[sarina]

If not please close.