[webkit-reviews] review denied: [Bug 176454] Implement CSS overscroll-behavior : [Attachment 399355] rebase

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied  review:
Bug 176454: Implement CSS overscroll-behavior
https://bugs.webkit.org/show_bug.cgi?id=176454

Attachment 399355: rebase

https://bugs.webkit.org/attachment.cgi?id=399355&action=review




--- Comment #36 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 399355
  --> https://bugs.webkit.org/attachment.cgi?id=399355
rebase

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

I don't see any changes here related to scroll latching, which I'd expect are
necessary to stop scroll propagation. For example, I would expect some code in
ScrollingTree::handleWheelEvent/ScrollingTreeLatchingController.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3044
> +	   case CSSPropertyOverscrollBehavior:
> +	       return
cssValuePool.createValue(std::max(style.overscrollBehaviorX(),
style.overscrollBehaviorY()));
> +	   case CSSPropertyOverscrollBehaviorX:
> +	       return cssValuePool.createValue(style.overscrollBehaviorX());
> +	   case CSSPropertyOverscrollBehaviorY:
> +	       return cssValuePool.createValue(style.overscrollBehaviorY());

This code should check if the feature is enabled, right?

> Source/WebCore/css/CSSPrimitiveValueMappings.h:2209
> +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(OverscrollBehavior e)

e -> behavior

> Source/WebCore/css/parser/CSSParserFastPaths.cpp:595
> +	   return valueID == CSSValueAuto || valueID == CSSValueContain ||
valueID == CSSValueNone;

Does this need to check overscrollBehaviorEnabled ?

> Source/WebCore/page/EventHandler.cpp:305
> +static bool didScrollInScrollableArea(ScrollableArea& scrollableArea, double
deltaX, double deltaY, unsigned deltaMode)

Can we make deltaMode be an enum first, please? It's hard to tell what it means
in this context.

> Source/WebCore/page/FrameView.cpp:428
> +    if (RenderWidget* frameRenderer = frame().ownerRenderer())
> +	   return frameRenderer->style().overscrollBehaviorX();

This is pretty weird; style in the parent document can affect overscroll in a
subframe (any subframe, possibly cross-origin?). That doesn't seem right.
Overscroll's impact should be limited to the current frame. I filed
https://github.com/w3c/csswg-drafts/issues/5129

Certainly this code is wrong because the parent frame's style may not have been
updated. The parent frame needs to push some state down onto this FrameView, if
this is something we need to support.