Bug 128688

Summary: During a copy, position:fixed gets converted to position:absolute even if only part of the document is selected
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: HTML EditingAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, jonlee, rniwa, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch rniwa: review+

Description Myles C. Maxfield 2014-02-12 14:05:30 PST
During a copy, position:fixed gets converted to position:absolute even if only part of the document is selected
Comment 1 Myles C. Maxfield 2014-02-12 14:13:29 PST
Created attachment 224005 [details]
Patch
Comment 2 Ryosuke Niwa 2014-02-12 14:16:36 PST
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.
Comment 3 Ryosuke Niwa 2014-02-12 14:55:06 PST
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-.
Comment 4 Ryosuke Niwa 2014-02-12 15:52:10 PST
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.
Comment 5 Myles C. Maxfield 2014-02-13 12:31:24 PST
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
Comment 6 Myles C. Maxfield 2014-02-13 13:48:34 PST
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.
Comment 7 Myles C. Maxfield 2014-02-13 13:52:29 PST
http://trac.webkit.org/changeset/164053
Comment 8 Radar WebKit Bug Importer 2014-02-24 18:42:26 PST
<rdar://problem/16156149>