Bug 155991 - [OS X] [RTL Scrollbars] List boxes should obey RTL scrollbars
Summary: [OS X] [RTL Scrollbars] List boxes should obey RTL scrollbars
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-29 15:54 PDT by Myles C. Maxfield
Modified: 2016-04-04 19:53 PDT (History)
6 users (show)

See Also:


Attachments
Patch (27.57 KB, patch)
2016-03-29 15:57 PDT, Myles C. Maxfield
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2016-03-29 15:54:56 PDT
[OS X] [RTL Scrollbars] List boxes should obey RTL scrollbars
Comment 1 Myles C. Maxfield 2016-03-29 15:57:50 PDT
Created attachment 275142 [details]
Patch
Comment 2 Darin Adler 2016-03-30 09:41:40 PDT
Comment on attachment 275142 [details]
Patch

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

> Source/WebCore/rendering/RenderListBox.cpp:476
> +    if (verticalScrollbarIsOnLeft() && (offset.width() < borderLeft() + paddingLeft() + scrollbarWidth || offset.width() > width() - borderRight() - paddingRight()))
> +        return -1;
> +    if (!verticalScrollbarIsOnLeft() && (offset.width() < borderLeft() + paddingLeft() || offset.width() > width() - borderRight() - paddingRight() - scrollbarWidth))
>          return -1;

Inelegant to have this in two separate if statements, but I have no cleaner suggestion.

> Source/WebCore/rendering/RenderListBox.cpp:721
> -        if (itemBoundingBoxRect(adjustedLocation, i).contains(locationInContainer.point())) {
> -            if (Element* node = listItems[i]) {
> -                result.setInnerNode(node);
> -                if (!result.innerNonSharedNode())
> -                    result.setInnerNonSharedNode(node);
> -                result.setLocalPoint(locationInContainer.point() - toLayoutSize(adjustedLocation));
> -                break;
> -            }
> +        if (!itemBoundingBoxRect(adjustedLocation, i).contains(locationInContainer.point()))
> +            continue;
> +        if (Element* node = listItems[i]) {
> +            result.setInnerNode(node);
> +            if (!result.innerNonSharedNode())
> +                result.setInnerNonSharedNode(node);
> +            result.setLocalPoint(locationInContainer.point() - toLayoutSize(adjustedLocation));
> +            break;
>          }

I like the new code slightly better, but why make this unrelated change in this patch.

> Source/WebCore/rendering/RenderListBox.cpp:905
>  bool RenderListBox::scrolledToTop() const
>  {
> -    Scrollbar* vbar = verticalScrollbar();
> -    if (!vbar)
> -        return true;
> -    
> +    if (Scrollbar* vbar = verticalScrollbar())
>      return vbar->value() <= 0;
> +
> +    return true;
>  }
>  
>  bool RenderListBox::scrolledToBottom() const
>  {
> -    Scrollbar* vbar = verticalScrollbar();
> -    if (!vbar)
> -        return true;
> +    if (Scrollbar* vbar = verticalScrollbar())
> +        return vbar->value() >= vbar->maximum();
>  
> -    return vbar->value() >= vbar->maximum();
> +    return true;
>  }

Old code with early return looks better to me.
Comment 3 Myles C. Maxfield 2016-03-30 10:34:31 PDT
Committed r198843: <http://trac.webkit.org/changeset/198843>
Comment 4 Myles C. Maxfield 2016-04-04 19:53:45 PDT
<rdar://problem/25137021>