Convert position:sticky and position:fixed properties to position:static and position:absolute upon copy
Created attachment 223168 [details] Patch
<rdar://problem/15696956>
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.
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.
> 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.
(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.
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
Created attachment 223795 [details] Patch
Created attachment 223797 [details] Patch
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.
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.
Created attachment 223889 [details] Patch
Related: https://bugs.webkit.org/show_bug.cgi?id=128616
http://trac.webkit.org/changeset/163916
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.
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. :-(