Bug 111360 – After sending message, Mail changes formatting

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 191302 [details]
Reduction

Reproduction steps:
1. Triple click the line "– 1. Copy this line after triple click" and copy
2. Paste it after inserting a new line at the end of "2. insert new line after this and paste, select all and copy"
3. Select all
4. Copy
5. Paste it in the box right beneath "3. paste it below:"

Expected result:
Source and Destination look identical

Actual result:
The line pasted in step 2 has a bigger font.

<rdar://problem/12442331>

[Ryosuke Niwa]

The problem is caused by us generating nested p's. when WebKit inserts the copied content in step 2 (pasting "1. Copy this line after triple click"), it creates the following markup:

<p class="primary_area" style="font-size: 12px;"><p><span class="bullet">–</span><span class="platforms">1. &nbsp;</span>Copy this line after triple click<br></p><div><br></div></p>

Here, p is nested so HTML5 tree builder will close the outer p "prematurely".

[Adam Barth]

I can reproduce this issue.

[Adam Barth]

> Here, p is nested so HTML5 tree builder will close the outer p "prematurely".

How is the tree builder involved in this operation?  For parsing the fragment somehow?

[Ryosuke Niwa]

(In reply to comment #3)
> > Here, p is nested so HTML5 tree builder will close the outer p "prematurely".
> 
> How is the tree builder involved in this operation?  For parsing the fragment somehow?

Right. I have an even better repro. that only require pasting once.

[Ryosuke Niwa]

Created attachment 191350 [details]
Reduced repro. that requires only one paste operation

[Adam Barth]

I don't fully understand what the editing code is doing here, but I think you're running up against a basic limitation of HTML which is that not every HTML DOM can be represented as an HTML string.  If your DOM has nested HTMLParagraphElements, you won't be able to represent that using an HTML string because the HTML parser automatically closes the first <p> when parsing the "nested" <p>.

[Adam Barth]

What that means is that you'll always be able to find examples of DOMs that cannot be round-tripped through strings via copy and paste.

[Ryosuke Niwa]

Created attachment 191377 [details]
Work in progress 1

[Ojan Vafai]

(In reply to comment #6)
> I don't fully understand what the editing code is doing here, but I think you're running up against a basic limitation of HTML which is that not every HTML DOM can be represented as an HTML string.  If your DOM has nested HTMLParagraphElements, you won't be able to represent that using an HTML string because the HTML parser automatically closes the first <p> when parsing the "nested" <p>.

(In reply to comment #7)
> What that means is that you'll always be able to find examples of DOMs that cannot be round-tripped through strings via copy and paste.

That sucks for editing. Maybe clipboard parsing on paste shouldn't go through the standard HTML parser since typically the things put on clipboards are done by the browser and not by web developers.

[Ryosuke Niwa]

(In reply to comment #9)
>
> (In reply to comment #7)
> > What that means is that you'll always be able to find examples of DOMs that cannot be round-tripped through strings via copy and paste.
> 
> That sucks for editing. Maybe clipboard parsing on paste shouldn't go through the standard HTML parser since typically the things put on clipboards are done by the browser and not by web developers.

Yeah, I initially though that might work 'til I realized that when this message is sent (on Mail, Gmail, etc…) to someone else, the recipient will use the regular HTML5 parsing algorithm to view the message :(

[Ryosuke Niwa]

Created attachment 191624 [details]
Work in progress 2

[Ryosuke Niwa]

Created attachment 191626 [details]
Current test failures

The current approach doesn't seem to work because of the cases where we want to preserve border lines and other styles in the outer block.

We need to instead let elements get nested and then remove them as needed after the insertion.

[Ryosuke Niwa]

Created attachment 191903 [details]
Work in progress 3

[Ryosuke Niwa]

Created attachment 191904 [details]
Before copy & paste (paste-text-018.html)

Right now, this patch arguably regresses the following 3 tests:
editing/pasteboard/line-feed-between-br-and-b-should-not-reorder-pasted-content.html
editing/pasteboard/paste-text-018.html
editing/pasteboard/testcase-9507.html

[Ryosuke Niwa]

Created attachment 191906 [details]
After copy & paste (paste-text-018.html)

And here are screenshots showing the behavior. In particular, the red box that surrounds two lines will surround only the first line after copy & paste.
But I'm not sure if this should be a blocker since the said tests aren't really testing whether the red boxes to stay.

If this is considered as a regression, then I don't see any other way but to abandon the current approach and add a postmortem fix up instead (probably into SimplifyMarkupCommand).

[Aryeh Gregor]

You definitely want to make sure that no editing actions create DOMs that don't round-trip through HTML serialization.  The editing spec says that at various points, newly-inserted nodes should be checked to see if they have prohibited ancestors, and if they do, they break out of their parent repeatedly until their parent is allowed:

https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#fix-disallowed-ancestors
https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#allowed-child

E.g., if you had <p>foo<p>bar</p>baz</p>, it would become <p>foo</p><p>bar</p><p>baz</p>.

There are more things to worry about than just nested <p> -- you also can't nest <h#> in each other, nor <a>, and using table-related elements incorrectly can be very exciting (they'll vanish or jump around).  If you care about corner cases, <xmp> is fun, and <plaintext> even more so.  The spec's algorithm also converts misplaced <li>/<dt>/<dd> to <p>/<div>, although that round-trips fine, and likewise doesn't allow block elements inside inline.

[Ryosuke Niwa]

Created attachment 192134 [details]
Fixes the bug

[WebKit Review Bot]

Comment on attachment 192134 [details]
Fixes the bug

Attachment 192134 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17021275

New failing tests:
editing/pasteboard/paste-text-016.html

[Ryosuke Niwa]

Created attachment 192160 [details]
Also prevent nested h1-h6

[WebKit Review Bot]

Comment on attachment 192160 [details]
Also prevent nested h1-h6

Attachment 192160 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17054402

New failing tests:
editing/pasteboard/paste-text-016.html

[Adam Barth]

Comment on attachment 192160 [details]
Also prevent nested h1-h6

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

Please do

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:462
> +    DEFINE_STATIC_LOCAL(HashSet<AtomicString>, elements, ());

Please do not add static state to the tree builder.  It's likely we'll want to move it onto a background thread at some point, and we can't have static state on a background thread.

> Source/WebCore/html/parser/HTMLTreeBuilder.h:92
> +    static bool isProhibitedParagraphChild(const AtomicString&);

I'd wrap this in an #ifndef NDEBUG to make it clear that it's a debug-only function.

[Adam Barth]

(In reply to comment #21)
> (From update of attachment 192160 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192160&action=review
> 
> Please do

^^^ Typo  :)

[Enrica Casucci]

Comment on attachment 192160 [details]
Also prevent nested h1-h6

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

> Source/WebCore/ChangeLog:26
> +        (ReplaceSelectionCommand):

Remove or add the list of added methods.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:562
> +// FIXME: Fix other types of DOM tree constructs like nested h1's that aren't isomortic under the HTML serialization and parsering algorithms.

typos: isomortic and parsering

[Ryosuke Niwa]

Created attachment 192257 [details]
Fixes the bug

[Enrica Casucci]

Comment on attachment 192257 [details]
Fixes the bug

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

Looks good to me, Please add the FIXME to makeInsertedContentRoundTrippableWithHTMLTreeBuilder and address the test failures on cr-linux

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:617
> +

You removed the comment with the FIXME:
I think you should add it back.

[Eric Seidel (no email)]

Comment on attachment 192160 [details]
Also prevent nested h1-h6

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

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:462
>> +    DEFINE_STATIC_LOCAL(HashSet<AtomicString>, elements, ());
> 
> Please do not add static state to the tree builder.  It's likely we'll want to move it onto a background thread at some point, and we can't have static state on a background thread.

In this particular case, I'm not sure this is a problem, or?  We'd have to initialize this at a known time, but then it should be threadsafe to read from this hash, no?

>> Source/WebCore/html/parser/HTMLTreeBuilder.h:92
>> +    static bool isProhibitedParagraphChild(const AtomicString&);
> 
> I'd wrap this in an #ifndef NDEBUG to make it clear that it's a debug-only function.

He's using it from the Editor. :)

[Adam Barth]

(In reply to comment #26)
> (From update of attachment 192160 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192160&action=review
> 
> >> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:462
> >> +    DEFINE_STATIC_LOCAL(HashSet<AtomicString>, elements, ());
> > 
> > Please do not add static state to the tree builder.  It's likely we'll want to move it onto a background thread at some point, and we can't have static state on a background thread.
> 
> In this particular case, I'm not sure this is a problem, or?  We'd have to initialize this at a known time, but then it should be threadsafe to read from this hash, no?

I'd rather not introduce static dependencies at all.

[Eric Seidel (no email)]

Comment on attachment 192257 [details]
Fixes the bug

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:561
> +bool isProhibitedParagraphChild(const AtomicString& name)

Is this where you're getting the list?
http://www.whatwg.org/specs/web-apps/current-work/multipage/tree-construction.html#parsing-main-inbody

[Eric Seidel (no email)]

(In reply to comment #28)
> (From update of attachment 192257 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192257&action=review
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:561
> > +bool isProhibitedParagraphChild(const AtomicString& name)
> 
> Is this where you're getting the list?
> http://www.whatwg.org/specs/web-apps/current-work/multipage/tree-construction.html#parsing-main-inbody

AKA this line in our treebuidler:
http://trac.webkit.org/browser/trunk/Source/WebCore/html/parser/HTMLTreeBuilder.cpp#L683

[Adam Barth]

I would like a chance to review this patch before it lands.

[Eric Seidel (no email)]

Comment on attachment 192257 [details]
Fixes the bug

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:563
> +    // https://dvcs.w3.org/hg/editing/raw-file/57abe6d3cb60/editing.html#prohibited-paragraph-child

If this list is based on an editing spec, then yes, this definitely belongs in Editing and not the tree-builder.  If this is trying to share spec space with the HTML5 treebuilder, then we could presumably share code.

[Ryosuke Niwa]

Committed r145253: <http://trac.webkit.org/changeset/145253>

[Adam Barth]

(In reply to comment #32)
> Committed r145253: <http://trac.webkit.org/changeset/145253>

Why did you land this patch after I explicitly asked to review it before it landed?

[Ryosuke Niwa]

(In reply to comment #33)
> (In reply to comment #32)
> > Committed r145253: <http://trac.webkit.org/changeset/145253>
> 
> Why did you land this patch after I explicitly asked to review it before it landed?

Sorry, I didn't see your comment.

[Ryosuke Niwa]

Are there any specific concerns you have with the patch? I'm more than happy to address them or roll out the patch if needed.

[Adam Barth]

Did you not see Eric's comments either?

[Ryosuke Niwa]

(In reply to comment #36)
> Did you not see Eric's comments either?

No. I was talking with him on IRC. He had a few suggestions but he told me he's also fine with having a list in editing so I went ahead with the current approach.

[Adam Barth]

(In reply to comment #35)
> Are there any specific concerns you have with the patch? I'm more than happy to address them or roll out the patch if needed.

The issue I wanted to study was where this list comes from and whether it is redundant with other tag lists we have already.  The current patch reads like a rushed hack that is unlikely to be maintained as HTML parsing evolves in the future.  If we can build on other lists that are maintained for other purposes, we'll have less maintenance overall.

I'm eating lunch right now, so I don't have a large enough screen to study this question in detail.

[Ryosuke Niwa]

(In reply to comment #38)
> The issue I wanted to study was where this list comes from and whether it is redundant with other tag lists we have already.

The list comes from https://dvcs.w3.org/hg/editing/raw-file/57abe6d3cb60/editing.html#prohibited-paragraph-child although I suspect Aryeh grabbed it from HTML5 specifications. See his comment #16.

Unfortunately, the editing code is not at the state to implement the fix-disallowed-ancestors algorithm in the specification.

> If we can build on other lists that are maintained for other purposes, we'll have less maintenance overall.

That'll be nice indeed.

In general, we have a problem of editing code getting outdated while new features are added to HTML/CSS. We also have a numerous list of elements that I suspect are already outdated or do not contain all necessary elements.

[Adam Barth]

> In general, we have a problem of editing code getting outdated while new features are added to HTML/CSS. We also have a numerous list of elements that I suspect are already outdated or do not contain all necessary elements.

It sounds like you're saying that the editing code is full of unmaintainable hacks and therefore you don't mind adding yet another one.  Whatever, man.  I'm sorry I tried to engage with you on this bug.

[Ryosuke Niwa]

(In reply to comment #40)
> > In general, we have a problem of editing code getting outdated while new features are added to HTML/CSS. We also have a numerous list of elements that I suspect are already outdated or do not contain all necessary elements.
> 
> It sounds like you're saying that the editing code is full of unmaintainable hacks and therefore you don't mind adding yet another one.  Whatever, man.  I'm sorry I tried to engage with you on this bug.

No, I'm not saying that. I'm saying that we need to fix it.

[Ryosuke Niwa]

That's why my initial patch attempted to add the list to HTMLTreeBuilder.cpp in the hope that people adding new stuff there could update the list.

However, you made it clear that you don't want a static variable in HTMLTreeBuilder. Given that, I see no other option but to add it to ReplaceSelectionCommand.

As I've repeatedly said, I'm all ears if someone can come up with a way to share lists with HTMLTreeBuilder since that's what I want as well but I don't think anyone has done that yet.

[Aryeh Gregor]

(In reply to comment #38)
> The issue I wanted to study was where this list comes from and whether it is redundant with other tag lists we have already.  The current patch reads like a rushed hack that is unlikely to be maintained as HTML parsing evolves in the future.  If we can build on other lists that are maintained for other purposes, we'll have less maintenance overall.

I whole-heartedly agree.  Unfortunately, the editing spec cares about a lot of stuff that nothing else in the platform does, because editing stuff deals with the DOM on a much higher level than anything else I'm aware of, so there's heavy dependency on editing-specific behavior for particular elements or lists thereof.  Things like introducing a new HTML element will need case-by-case consideration for what their impact on editing should be, in general (if the element is relevant to editable content).

Specifically, in this case, in the spec

https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#prohibited-paragraph-child-name

you'll find "View comments" links at the side.  The comment here says

"""
These are all the things that will close a <p> if found as a descendant. I think. Plus table stuff, since that can't be a descendant of a p either, although it won't auto-close it.
"""

So that tells you how I made the list.  I don't think it's redundant to anything in the HTML specification or can be constructed automatically from anything in the specification.  But if you have any suggestions to reduce redundancy or reduce the chances of the specs falling out of sync, I'd be very interested to know.