Bug 83714 – Resolve styles when pasting HTML

• Status: NEW
• Component: HTML Editing

[Daniel Cheng]

http://crbug.com/121163

It appears that Firefox and IE appear to resolve styles in <style> blocks correctly when inserting HTML. We should try to figure out how to do the same with WebKit.

This last time this came up, I tried two similar approaches:
1) Detached Document with EmptyClients (this was considered a hack and abandoned)
2) Having the embedder supply a Page. I actually got this working at one point, but it had the same problem with asserts that approach #1 did (https://bugs.webkit.org/show_bug.cgi?id=62112#c34 illustrates some of the issues I hit last time).

#1 is probably out of the question, but #2 might still be worth considering.

[Ryosuke Niwa]

(In reply to comment #0)
> It appears that Firefox and IE appear to resolve styles in <style> blocks correctly when inserting HTML. We should try to figure out how to do the same with WebKit.
> 
> This last time this came up, I tried two similar approaches:
> 1) Detached Document with EmptyClients (this was considered a hack and abandoned)
> 2) Having the embedder supply a Page. I actually got this working at one point, but it had the same problem with asserts that approach #1 did (https://bugs.webkit.org/show_bug.cgi?id=62112#c34 illustrates some of the issues I hit last time).

Given that we have a support for scoped content attribute, we might be able to do:
1. Paste style element with the rest of contents but with "scoped" content attribute
2. Serialize styles from the scoped style elements as inline style declarations
3. Get rid of the scoped style elements.

[Daniel Cheng]

Created attachment 141831 [details]
Experimental patch. What do you think of the approach in this patch? I'll clean it up for landing if people think it's acceptable.

[Build Bot]

Comment on attachment 141831 [details]
Experimental patch. What do you think of the approach in this patch? I'll clean it up for landing if people think it's acceptable.

Attachment 141831 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12703194

[Early Warning System Bot]

Comment on attachment 141831 [details]
Experimental patch. What do you think of the approach in this patch? I'll clean it up for landing if people think it's acceptable.

Attachment 141831 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12704140

[Gustavo Noronha (kov)]

Comment on attachment 141831 [details]
Experimental patch. What do you think of the approach in this patch? I'll clean it up for landing if people think it's acceptable.

Attachment 141831 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12682552

[Early Warning System Bot]

Comment on attachment 141831 [details]
Experimental patch. What do you think of the approach in this patch? I'll clean it up for landing if people think it's acceptable.

Attachment 141831 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12705136

[Build Bot]

Comment on attachment 141831 [details]
Experimental patch. What do you think of the approach in this patch? I'll clean it up for landing if people think it's acceptable.

Attachment 141831 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12705132

[Gyuyoung Kim]

Comment on attachment 141831 [details]
Experimental patch. What do you think of the approach in this patch? I'll clean it up for landing if people think it's acceptable.

Attachment 141831 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12684600

[Ryosuke Niwa]

Comment on attachment 141831 [details]
Experimental patch. What do you think of the approach in this patch? I'll clean it up for landing if people think it's acceptable.

View in context: https://bugs.webkit.org/attachment.cgi?id=141831&action=review

> Source/WebCore/editing/markup.cpp:771
> +    // Move any orphaned style nodes into the trimmed fragment and force them to be scoped.
> +    // FIXME: Presumably we only want to include things that aren't scoped and were in head.
> +    for (size_t i = 0 ; i < styles.size(); ++i)
> +        if (!styles[i]->isDescendantOf(fragment.get())) {
> +            fragment->insertBefore(styles[i], fragment->firstChild());
> +            static_cast<HTMLStyleElement*>(styles[i].get())->setScoped(true);
> +        }

This won't work as most of UAs don't support scoped style elements. An important requirement for editing is that whatever markup we generate must be rendered properly by old UAs because they might get sent by emails, etc...

[Ryosuke Niwa]

Comment on attachment 141831 [details]
Experimental patch. What do you think of the approach in this patch? I'll clean it up for landing if people think it's acceptable.

r- to that end.

[Ryosuke Niwa]

Comment on attachment 141831 [details]
Experimental patch. What do you think of the approach in this patch? I'll clean it up for landing if people think it's acceptable.

View in context: https://bugs.webkit.org/attachment.cgi?id=141831&action=review

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:604
> +            || (node->hasTagName(styleTag)
> +                && !static_cast<HTMLStyleElement*>(node)->scoped())) {

This won't quite work as blocks can get merged, purged, etc... as we edit contents.

[Daniel Cheng]

Comment on attachment 141831 [details]
Experimental patch. What do you think of the approach in this patch? I'll clean it up for landing if people think it's acceptable.

View in context: https://bugs.webkit.org/attachment.cgi?id=141831&action=review

>> Source/WebCore/editing/markup.cpp:771
>> +        }
> 
> This won't work as most of UAs don't support scoped style elements. An important requirement for editing is that whatever markup we generate must be rendered properly by old UAs because they might get sent by emails, etc...

I think this is actually OK--we simply shouldn't do these steps on UAs that don't support scoped style elements. When the resulting markup is copied or dragged out, we will correctly serialize the styles to be inline so it should work.

[Ryosuke Niwa]

Comment on attachment 141831 [details]
Experimental patch. What do you think of the approach in this patch? I'll clean it up for landing if people think it's acceptable.

View in context: https://bugs.webkit.org/attachment.cgi?id=141831&action=review

>>> Source/WebCore/editing/markup.cpp:771
>>> +        }
>> 
>> This won't work as most of UAs don't support scoped style elements. An important requirement for editing is that whatever markup we generate must be rendered properly by old UAs because they might get sent by emails, etc...
> 
> I think this is actually OK--we simply shouldn't do these steps on UAs that don't support scoped style elements. When the resulting markup is copied or dragged out, we will correctly serialize the styles to be inline so it should work.

No, whatever markup we generate will be saved on a server and will be sent to all kinds of UAs. So, no, it is not okay.

[Daniel Cheng]

Ah, I see what you mean.

In that case, we'd have to do a followup pass to collapse any styles in place in the inserted block... would that be OK? I know we have some code that already does that in markup.cpp for the opposite case (converting a fragment to markup). However, I think it might inline too much style information (for example, if we're just pasting a word, we probably shouldn't inline styles inherited from body, etc), so we'd need to adapt it.

[Ryosuke Niwa]

(In reply to comment #14)
> In that case, we'd have to do a followup pass to collapse any styles in place in the inserted block... would that be OK? I know we have some code that already does that in markup.cpp for the opposite case (converting a fragment to markup). However, I think it might inline too much style information (for example, if we're just pasting a word, we probably shouldn't inline styles inherited from body, etc), so we'd need to adapt it.

Right, and StylizedMarkupAccumulator already knows how to do that. It's a matter of refactoring that code (mostly in EditingStyle now) and make it usable in ReplaceSelectionCommand.

[Ryosuke Niwa]

Here's an idea. You can insert an iframe in which you put the pasted content, resolve all inlines as inlines style declarations, and then create a document fragment from that.

We can hide the iframe by not generating mutation events, and not including it in mutation records so nobody would know the existence of this iframe.

[Kenji Baheux]

Daniel, do you have the bandwidth to follow through with Ryosuke's idea? Are there still unresolved issues even with this proposal?

Thanks a lot.

(In reply to comment #16)
> Here's an idea. You can insert an iframe in which you put the pasted content, resolve all inlines as inlines style declarations, and then create a document fragment from that.
> 
> We can hide the iframe by not generating mutation events, and not including it in mutation records so nobody would know the existence of this iframe.

[Daniel Cheng]

(In reply to comment #17)
> Daniel, do you have the bandwidth to follow through with Ryosuke's idea? Are there still unresolved issues even with this proposal?
> 
> Thanks a lot.
> 
> (In reply to comment #16)
> > Here's an idea. You can insert an iframe in which you put the pasted content, resolve all inlines as inlines style declarations, and then create a document fragment from that.
> > 
> > We can hide the iframe by not generating mutation events, and not including it in mutation records so nobody would know the existence of this iframe.

I don't have the bandwidth to follow up on this at the moment; this is probably something I'd be looking at in a few months if no one else had picked it up by then. If you want to fix this, feel free to take the bug =)

[Ahmad Saleem]

https://chromium.googlesource.com/chromium/src.git/+/8ebfae3567b1bfe350286345bb0c338b9f354fe1

^ to explore (if possible to merge)