Bug 128194 – Convert position:sticky and position:fixed properties to position:static and position:absolute upon copy

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Convert position:sticky and position:fixed properties to position:static and position:absolute upon copy

[Myles C. Maxfield]

Created attachment 223168 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/15696956>

[Simon Fraser (smfr)]

Comment on attachment 223168 [details]
Patch

It's not clear to me that we want to do this unconditionally on copy; you'll break lots of HTML editors. Seems like the recipient of the paste needs to do the mungeing.

[Ryosuke Niwa]

Comment on attachment 223168 [details]
Patch

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

> Source/WebCore/editing/EditingStyle.cpp:1232
> +    if (m_mutableStyle->propertyMatches(CSSPropertyPosition, sticky.get()))
> +        m_mutableStyle->setProperty(CSSPropertyPosition, cssValuePool().createIdentifierValue(CSSValueStatic), false);

We should probably preserve the importance of the property.

> Source/WebCore/editing/markup.cpp:322
> +            
> +            newInlineStyle->convertFixedAndStickyPosition();

We need this function to return a boolean and keep that state in StyledMarkupAccumulator
so that we can remember to wrap the whole thing with position:relative block.

Also, Simon is right in that we don't want to do this unless we're copying the entire document
so we need the constructor of StylizedMarkupAccumulator to take some enum/boolean for that as well.

[Simon Fraser (smfr)]

> Also, Simon is right in that we don't want to do this unless we're copying the entire document
Why? I still think this should be a paste-time filter.

[Ryosuke Niwa]

(In reply to comment #5)
> > Also, Simon is right in that we don't want to do this unless we're copying the entire document
> Why? I still think this should be a paste-time filter.

No, at the paste time, we don't know where elements are placed.  Also, copy-time is where we do all these style serializations.

We used to have some code in paste side trying to fix the content and that was an epic fail. We've moved most of that stuff into copy site over time.

[Myles C. Maxfield]

Comment on attachment 223168 [details]
Patch

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

>> Source/WebCore/editing/EditingStyle.cpp:1232
>> +        m_mutableStyle->setProperty(CSSPropertyPosition, cssValuePool().createIdentifierValue(CSSValueStatic), false);
> 
> We should probably preserve the importance of the property.

Done.

>> Source/WebCore/editing/markup.cpp:322
>> +            newInlineStyle->convertFixedAndStickyPosition();
> 
> We need this function to return a boolean and keep that state in StyledMarkupAccumulator
> so that we can remember to wrap the whole thing with position:relative block.
> 
> Also, Simon is right in that we don't want to do this unless we're copying the entire document
> so we need the constructor of StylizedMarkupAccumulator to take some enum/boolean for that as well.

Done. I'm doing that check in createMarkupInternal

[Myles C. Maxfield]

Created attachment 223795 [details]
Patch

[Myles C. Maxfield]

Created attachment 223797 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 223795 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        block (position:relative) is wrapped around the copied text. If there is a position:-webkit-sticky
> +        element, it is replaced with position:static.

You should convert sticky to position:relative, so that positioned descendants stay more or less in the same place.

[Myles C. Maxfield]

Comment on attachment 223795 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        element, it is replaced with position:static.
> 
> You should convert sticky to position:relative, so that positioned descendants stay more or less in the same place.

Done.

[Myles C. Maxfield]

Created attachment 223889 [details]
Patch

[Myles C. Maxfield]

Related: https://bugs.webkit.org/show_bug.cgi?id=128616

[Myles C. Maxfield]

http://trac.webkit.org/changeset/163916

[Ryosuke Niwa]

Comment on attachment 223889 [details]
Patch

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

> Source/WebCore/editing/markup.cpp:330
> +            if (element.document().settings() && element.document().settings()->convertPositionStyleOnCopy())
> +                m_needRelativeStyleWrapper |= newInlineStyle->convertFixedAndStickyPosition();

There's a bug here. This is isn't checking thatfullySelectedRoot == body was true in createMarkupInternal :(
As such, this code runs even if the entire body wasn't selected.

[Myles C. Maxfield]

Comment on attachment 223889 [details]
Patch

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

>> Source/WebCore/editing/markup.cpp:330
>> +                m_needRelativeStyleWrapper |= newInlineStyle->convertFixedAndStickyPosition();
> 
> There's a bug here. This is isn't checking thatfullySelectedRoot == body was true in createMarkupInternal :(
> As such, this code runs even if the entire body wasn't selected.

Oh man. This was fixed in an earlier version of the patch, but must have gotten lost in refactoring. :-(