Bug 210673 - Horizontal overflow overlay scrollbar is misplaced in RTL
Summary: Horizontal overflow overlay scrollbar is misplaced in RTL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Compositing (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-17 14:33 PDT by Simon Fraser (smfr)
Modified: 2020-04-21 11:17 PDT (History)
10 users (show)

See Also:


Attachments
See the third box. turn layer borders on (1.13 KB, text/html)
2020-04-17 14:33 PDT, Simon Fraser (smfr)
no flags Details
Screenshot (78.09 KB, image/png)
2020-04-17 14:33 PDT, Simon Fraser (smfr)
no flags Details
Patch (34.04 KB, patch)
2020-04-20 22:39 PDT, Simon Fraser (smfr)
koivisto: review+
Details | Formatted Diff | Diff
Patch (33.28 KB, patch)
2020-04-21 09:32 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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