Bug 128688 – During a copy, position:fixed gets converted to position:absolute even if only part of the document is selected

• Component: HTML Editing
• Status: RESOLVED FIXED

[Myles C. Maxfield]

During a copy, position:fixed gets converted to position:absolute even if only part of the document is selected

[Myles C. Maxfield]

Created attachment 224005 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 224005 [details]
Patch

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

> Source/WebCore/editing/markup.cpp:627
> -    if (accumulator.needRelativeStyleWrapper() && body && fullySelectedRoot == body) {
> +    if (accumulator.needRelativeStyleWrapper() && needsPositionStyleConversion) {

I don't think we should do this. This would result in another round of Apple-style-span nightmare.

[Ryosuke Niwa]

Comment on attachment 224005 [details]
Patch

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

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

This is actually broken because convertFixedAndStickyPosition doesn't adjust coordinates with respect to the wrapping div's. r-.

[Ryosuke Niwa]

Comment on attachment 224005 [details]
Patch

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

>> Source/WebCore/editing/markup.cpp:328
>>                  m_needRelativeStyleWrapper |= newInlineStyle->convertFixedAndStickyPosition();
> 
> This is actually broken because convertFixedAndStickyPosition doesn't adjust coordinates with respect to the wrapping div's. r-.

Oh oops, I completely misunderstood this patch :(

I think we should check settings in createMarkupInternal though.

> Source/WebCore/editing/markup.cpp:570
> +    bool needsPositionStyleConversion = body && fullySelectedRoot == body;

We don't need this separate boolean because fullySelectedRoot can only ever set to body.

>> Source/WebCore/editing/markup.cpp:627
>> +    if (accumulator.needRelativeStyleWrapper() && needsPositionStyleConversion) {
> 
> I don't think we should do this. This would result in another round of Apple-style-span nightmare.

Ignore this comment.

[Myles C. Maxfield]

Comment on attachment 224005 [details]
Patch

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

>> Source/WebCore/editing/markup.cpp:570
>> +    bool needsPositionStyleConversion = body && fullySelectedRoot == body;
> 
> We don't need this separate boolean because fullySelectedRoot can only ever set to body.

I'm going to keep the separate boolean because it gets read twice, and it is conceptually different than the comparison that it gets set to

[Myles C. Maxfield]

Comment on attachment 224005 [details]
Patch

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

>>> Source/WebCore/editing/markup.cpp:328
>>>                  m_needRelativeStyleWrapper |= newInlineStyle->convertFixedAndStickyPosition();
>> 
>> This is actually broken because convertFixedAndStickyPosition doesn't adjust coordinates with respect to the wrapping div's. r-.
> 
> Oh oops, I completely misunderstood this patch :(
> 
> I think we should check settings in createMarkupInternal though.

Done.

[Myles C. Maxfield]

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

[Radar WebKit Bug Importer]

<rdar://problem/16156149>