Bug 210673

Summary: Horizontal overflow overlay scrollbar is misplaced in RTL
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CompositingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: esprehn+autocc, ews-watchlist, fred.wang, glenn, koivisto, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
See the third box. turn layer borders on
none
Screenshot
none
Patch
koivisto: review+
Patch none

Description Simon Fraser (smfr) 2020-04-17 14:33:06 PDT
Created attachment 396801 [details]
See the third box. turn layer borders on

We put the horizontal scrollbar in the wrong place.
Comment 1 Simon Fraser (smfr) 2020-04-17 14:33:32 PDT
Created attachment 396802 [details]
Screenshot
Comment 2 Radar WebKit Bug Importer 2020-04-17 14:33:46 PDT
<rdar://problem/61950751>
Comment 3 Simon Fraser (smfr) 2020-04-20 22:39:13 PDT
Created attachment 397055 [details]
Patch
Comment 4 Antti Koivisto 2020-04-21 02:16:16 PDT
Comment on attachment 397055 [details]
Patch

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

r+ but please consider the comment.

> Source/WebCore/rendering/RenderLayer.cpp:3185
> +    IntRect hBarRect, vBarRect, cornerRect, resizerRect;
> +    getOverflowControlsRects(hBarRect, vBarRect, cornerRect, resizerRect);

I'd much prefer a function that returns a struct over one that requires awkward out parameters.
Comment 5 Simon Fraser (smfr) 2020-04-21 09:32:59 PDT
Created attachment 397090 [details]
Patch
Comment 6 Antti Koivisto 2020-04-21 09:42:29 PDT
Comment on attachment 397055 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:3364
> +    auto cornerRect = [](const IntRect& boundsRect, IntSize cornerSize, bool placeVerticalScrollbarOnTheLeft) {
> +        if (placeVerticalScrollbarOnTheLeft) {
> +            auto bottomLeftCorner = boundsRect.minXMaxYCorner();
> +            return IntRect { { bottomLeftCorner.x(), bottomLeftCorner.y() - cornerSize.height(), }, cornerSize };
> +        }
> +        return IntRect { boundsRect.maxXMaxYCorner() - cornerSize, cornerSize };
> +    };
> +
> +    if (scrollbarsAvoidCorner)
> +        scrollCornerRect = cornerRect(overflowControlsPositioningRect, cornerSize, placeVerticalScrollbarOnTheLeft);
> +    else
> +        scrollCornerRect = { };

You could include 'if (!scrollbarsAvoidCorner)' branch into the lambda as well. 
Not sure this single use lambda really needs arguments at all, it could just capture the variables: [&] {

Other branches in this function would probably benefit from similar use of lambdas.
Comment 7 Simon Fraser (smfr) 2020-04-21 11:17:28 PDT
https://trac.webkit.org/changeset/260445/webkit