[webkit-reviews] review denied: [Bug 90046] Implement sticky positioning : [Attachment 160291] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied  review:
Bug 90046: Implement sticky positioning
https://bugs.webkit.org/show_bug.cgi?id=90046

Attachment 160291: Patch
https://bugs.webkit.org/attachment.cgi?id=160291&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=160291&action=review


Very excited to see this go in!

What should happen when a sticky positioned item is placed in another sticky
positioned item? Mind adding a test or two for this?

What's the plan for getting this into an editor's draft spec? Is Edward working
on that? Having concrete spec text might help get more concrete feedback from
other vendors.

> Source/WebCore/ChangeLog:4
> +	   Implement sticky positioning
> +	   https://bugs.webkit.org/show_bug.cgi?id=90046

This description needs updating (e.g. has collectMatchingRulesForList) and
claims to add isInFlowPositioned.

> Source/WebCore/page/FrameView.cpp:1505
> +	   if (renderer->style()->position() != FixedPosition &&
renderer->style()->position() != StickyPosition)

There are 4 places where we check if something is fixed or sticky. I wonder if
it makes sense to come up with a name for this and add an accessor to
RenderStyle in the way that we already have hasOutOfFlowPosition and
hasInFlowPosition.

hasViewportRelativePosition? hasViewportContainedPosition?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:563
> +    // Have to recompute the margins, because the already-computed margins
include extra space.

Can you clarify this comment a bit? Extra space from what? Is this just in the
case of margin: auto? If so, I think it'd be better to having something like
the following:
LayoutUnit leftMargin = style()->marginLeft().auto() ? 0 : marginLeft();

Then it's clear that you're special casing auto margins and you don't even need
this explanatory comment.

That said, it's not even clear to me why we'd want to do this. Wouldn't you
want margin: auto to apply as normal?

Can you add a test-case that covers this?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:567
> +    LayoutUnit minLeftMargin = minimumValueForLength(style()->marginLeft(),
containingBlock->availableWidth(), view());
> +    LayoutUnit minTopMargin = minimumValueForLength(style()->marginTop(),
containingBlock->availableHeight(), view());
> +    LayoutUnit minRightMargin =
minimumValueForLength(style()->marginRight(),
containingBlock->availableWidth(), view());
> +    LayoutUnit minBottomMargin =
minimumValueForLength(style()->marginBottom(),
containingBlock->availableHeight(), view());

The available size of margins is always based off the logical width (see, for
example, computeMargin in RenderInline.cpp). So, contrary to earlier comments,
all four of these should be using containingBlock->availableLogicalWidth(). :)

> Source/WebCore/rendering/RenderBoxModelObject.cpp:581
> +    // We can't call localToAbsolute on |this| because that will recur. For
now, assume that |this| is not transformed.

Should this be a FIXME to make transforms work on sticky positioned elements?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:593
> +	   if (absoluteStickyBoxRect.maxX() > absContainerContentRect.maxX())
> +	       absoluteStickyBoxRect.setX(absContainerContentRect.maxX() -
absoluteStickyBoxRect.width());

This seems like it'd do the wrong thing if the position:sticky item is wider
than its containing block? Actually, what is the right thing to do in that
case? Mind adding a test-case that covers this? Ditto for the right/top/bottom
cases below.