Bug 155991 – [OS X] [RTL Scrollbars] List boxes should obey RTL scrollbars

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[OS X] [RTL Scrollbars] List boxes should obey RTL scrollbars

[Myles C. Maxfield]

Created attachment 275142 [details]
Patch

[Darin Adler]

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.

[Myles C. Maxfield]

Committed r198843: <http://trac.webkit.org/changeset/198843>

[Myles C. Maxfield]

<rdar://problem/25137021>