REGRESSION (r172417, r184065): Multiple rendering issues with fixed attached background-image

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Repro steps:

1. Open the attached test case background-size-cover.html
2. Pinch-zoom

Result: the background is not displayed correctly. Mostly it is mot visible.
NOTE: The same result can be seen when loading americanmccarver.com and pinch-zooming.

1. Launch Safari.
2. Open the attached test case fixed-gradient-background.html.
3. Click and hold the window maximize button.
4. Select another application to share the screen with Safari
5. Resize the width io Safari window to the minimum

Result: The background-image does not cover the whole element rect.

1. Launch Safari.
2. Open the attached test case fixed-root-background.html.
3. Click and hold the window maximize button.
4. Select another application to share the screen with Safari
5. Resize the width io Safari window to the minimum

Result: The background-image does not cover the whole page.
NOTE: The same result when loading iCloud.com and have it in scaled full screen mode.

[Said Abou-Hallawa]

Created attachment 256986 [details]
background-size-cover.html

[Said Abou-Hallawa]

Created attachment 256987 [details]
fixed-gradient-background.html

[Said Abou-Hallawa]

Created attachment 256988 [details]
fixed-root-background.html

[Said Abou-Hallawa]

Created attachment 256995 [details]
Patch

[Dean Jackson]

Comment on attachment 256995 [details]
Patch

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

This looks ok to me, but Simon should review it.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1119
> +            // Fixed background-image on the root element

Nits: need . on all these comments.

[Said Abou-Hallawa]

Created attachment 257012 [details]
Patch

[Daniel Bates]

Comment on attachment 257012 [details]
Patch

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

I have not reviewed this patch for correctness. I noticed some minor nits.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1121
> +

Minor: Please remove this empty line as I do not feel it improves the readability of this code as the difference in indentation level is sufficient to demarcate the if-statement from the body of the if-statement.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1131
> +                // Scaled page.

Minor: The text in this comment can be inferred from the code below. Either elaborate further or remove this comment.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1141
> +                // Fixed layout size.

Minor: This comment reiterates what the code below does. Please remove this comment.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1143
> +                    // Expand viewportRect to borderBoxRect.

Minor: Similiarly this comment explain what the code below does, but does not explain why we should expand the viewport rectangle to the border box rectangle. We should either explain why we need to do this or we should remove this comment.

[Daniel Bates]

<rdar://problem/21110936>

[Simon Fraser (smfr)]

Comment on attachment 257012 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1123
> +                if (!frameView.fixedLayoutSize().isEmpty() && frameView.useFixedLayout())

I think you should flip the checks (ideally useFixedLayout() should ASSERT that the layout size is non-empty, and return false if it is.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1128
> +                    topContentInset = frameView.topContentInset(ScrollView::TopContentInsetType::WebCoreOrPlatformContentInset);
> +                    viewportRect.setLocation(LayoutPoint(0, -topContentInset));
> +                    viewportRect.setSize(frameView.unscaledVisibleContentSizeIncludingObscuredArea());

This logic seems to behave correctly.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1136
> +                    topContentInset = frameView.topContentInset(ScrollView::TopContentInsetType::WebCoreOrPlatformContentInset);
> +                    viewportRect.setLocation(toLayoutPoint(frameView.documentScrollOffsetRelativeToViewOrigin()));

This bit is correct.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1139
> +                viewportRect.setSize(frameView.unscaledVisibleContentSizeIncludingObscuredArea());

I don't think the scaling behavior is quite correct when zoomed, but it's better than it was, and matches the odd behavior of position:fixed when zoomed, so it's OK for now.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1142
> +                if (!frameView.fixedLayoutSize().isEmpty() && frameView.useFixedLayout()) {

Flip the checks.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1148
> +                    if (style().overflowX() != OHIDDEN)
> +                        viewportRect.setWidth(std::max(viewportRect.width(), borderBoxRect.width()));
> +
> +                    if (style().overflowY() != OHIDDEN)
> +                        viewportRect.setHeight(std::max(viewportRect.height(), borderBoxRect.height()));

I don't think this part is correct. You can see this by comparing two testcases, one with a fixed background on the body, and one with a fixed background on a 100%x100% div (which hits this code). In the div case, when hitting the fixed layout size threshold as the window is getting narrower, there's an obvious breakpoint where the background size changes. This shouldn't happen.

> Source/WebCore/rendering/RenderLayerBacking.cpp:928
> -            backgroundSize = frameView.visibleContentRect().size();
> +            backgroundSize = frameView.layoutSize();

This seems to work correctly.

[Said Abou-Hallawa]

Created attachment 257100 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 257012 [details]
Patch

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

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:1121
>> +
> 
> Minor: Please remove this empty line as I do not feel it improves the readability of this code as the difference in indentation level is sufficient to demarcate the if-statement from the body of the if-statement.

The empty line was removed.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:1123
>> +                if (!frameView.fixedLayoutSize().isEmpty() && frameView.useFixedLayout())
> 
> I think you should flip the checks (ideally useFixedLayout() should ASSERT that the layout size is non-empty, and return false if it is.

Clauses in the if-statment were flipped. I added the assertion you suggested in useFixedLayout() but it fires while resizing the window. Either its is a valid case to have useFixedLayout() is false and fixedLayoutSize() is non empty. Or the design of the code is fixedLayoutSize() should not be accessed at all unless useFixedLayout() is true. I have not checked.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:1131
>> +                // Scaled page.
> 
> Minor: The text in this comment can be inferred from the code below. Either elaborate further or remove this comment.

Comments were added.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:1141
>> +                // Fixed layout size.
> 
> Minor: This comment reiterates what the code below does. Please remove this comment.

The comment and the condition were removed.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:1142
>> +                if (!frameView.fixedLayoutSize().isEmpty() && frameView.useFixedLayout()) {
> 
> Flip the checks.

The condition is removed. I think we should always render to the maximum of the viewport and and the renderBox regardless whether we are in the fixedLayoutSize mode or not.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:1143
>> +                    // Expand viewportRect to borderBoxRect.
> 
> Minor: Similiarly this comment explain what the code below does, but does not explain why we should expand the viewport rectangle to the border box rectangle. We should either explain why we need to do this or we should remove this comment.

The comment was removed.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:1148
>> +                        viewportRect.setHeight(std::max(viewportRect.height(), borderBoxRect.height()));
> 
> I don't think this part is correct. You can see this by comparing two testcases, one with a fixed background on the body, and one with a fixed background on a 100%x100% div (which hits this code). In the div case, when hitting the fixed layout size threshold as the window is getting narrower, there's an obvious breakpoint where the background size changes. This shouldn't happen.

The condition for fixedLayoutSize() was removed so we should not have a breakpoint when the viewport size changes.

[Said Abou-Hallawa]

Created attachment 257144 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #11)
> Comment on attachment 257012 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257012&action=review
> 
> >> Source/WebCore/rendering/RenderBoxModelObject.cpp:1121
> >> +
> > 
> > Minor: Please remove this empty line as I do not feel it improves the readability of this code as the difference in indentation level is sufficient to demarcate the if-statement from the body of the if-statement.
> 
> The empty line was removed.
> 
> >> Source/WebCore/rendering/RenderBoxModelObject.cpp:1123
> >> +                if (!frameView.fixedLayoutSize().isEmpty() && frameView.useFixedLayout())
> > 
> > I think you should flip the checks (ideally useFixedLayout() should ASSERT that the layout size is non-empty, and return false if it is.
> 
> Clauses in the if-statment were flipped. I added the assertion you suggested
> in useFixedLayout() but it fires while resizing the window. Either its is a
> valid case to have useFixedLayout() is false and fixedLayoutSize() is non
> empty. Or the design of the code is fixedLayoutSize() should not be accessed
> at all unless useFixedLayout() is true. I have not checked.
> 
> >> Source/WebCore/rendering/RenderBoxModelObject.cpp:1131
> >> +                // Scaled page.
> > 
> > Minor: The text in this comment can be inferred from the code below. Either elaborate further or remove this comment.
> 
> Comments were added.
> 
> >> Source/WebCore/rendering/RenderBoxModelObject.cpp:1141
> >> +                // Fixed layout size.
> > 
> > Minor: This comment reiterates what the code below does. Please remove this comment.
> 
> The comment and the condition were removed.
> 
> >> Source/WebCore/rendering/RenderBoxModelObject.cpp:1142
> >> +                if (!frameView.fixedLayoutSize().isEmpty() && frameView.useFixedLayout()) {
> > 
> > Flip the checks.
> 
> The condition is removed. I think we should always render to the maximum of
> the viewport and and the renderBox regardless whether we are in the
> fixedLayoutSize mode or not.
> 
> >> Source/WebCore/rendering/RenderBoxModelObject.cpp:1143
> >> +                    // Expand viewportRect to borderBoxRect.
> > 
> > Minor: Similiarly this comment explain what the code below does, but does not explain why we should expand the viewport rectangle to the border box rectangle. We should either explain why we need to do this or we should remove this comment.
> 
> The comment was removed.
> 
> >> Source/WebCore/rendering/RenderBoxModelObject.cpp:1148
> >> +                        viewportRect.setHeight(std::max(viewportRect.height(), borderBoxRect.height()));
> > 
> > I don't think this part is correct. You can see this by comparing two testcases, one with a fixed background on the body, and one with a fixed background on a 100%x100% div (which hits this code). In the div case, when hitting the fixed layout size threshold as the window is getting narrower, there's an obvious breakpoint where the background size changes. This shouldn't happen.
> 
> The condition for fixedLayoutSize() was removed so we should not have a
> breakpoint when the viewport size changes.

This logic about expanding the background-image destinationRect to the borderBox in the case of useFixedLayoutSize() was wrong. We should use FrameView::fixedLayoutSize() all the times.

[WebKit Commit Bot]

Comment on attachment 257144 [details]
Patch

Clearing flags on attachment: 257144

Committed r187116: <http://trac.webkit.org/changeset/187116>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.