Bug 155533 – [RTL Scrollbars] Position: absolute divs are covered by vertical scrollbar

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Position: absolute needs to be moved over by the width of the scrollbar.

[Myles C. Maxfield]

This occurs with overflow-scroll and iframes.

[Myles C. Maxfield]

https://bugs.webkit.org/show_bug.cgi?id=155531 adds some disabled tests which test this.

[Myles C. Maxfield]

Created attachment 274255 [details]
Patch

[Myles C. Maxfield]

The relevant code is in RenderBox::computePositionedLogicalWidthUsing()

[Myles C. Maxfield]

The width is affected by scrollbar width in RenderBox::containingBlockLogicalWidthForPositioned(). This finds the width by calling containingBlock->clientLogicalWidth() (which takes scrollbar width into account).

[Myles C. Maxfield]

Created attachment 274468 [details]
WIP

[Myles C. Maxfield]

Created attachment 274531 [details]
Patch

[Darin Adler]

Comment on attachment 274531 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3526
> +        const RenderBox& containingBlock = downcast<RenderBox>(*containerBlock);

How about containingBox or box rather than containingBlock for this? Also, I suggest auto& instead of saying RenderBox twice on this line.

[Myles C. Maxfield]

Comment on attachment 274531 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3528
> +            computedValues.m_position += containingBlock.verticalScrollbarWidth();

This doesn't work with iframes, because the scrollbar width for an iframe comes from the iframe's RenderView's FrameView (which is a ScrollableArea)

I need to do more investigation before landing this. It seems likely that the best way to fix iframes is in another part of WebKit (not here)

[Myles C. Maxfield]

Comment on attachment 274531 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:3528
>> +            computedValues.m_position += containingBlock.verticalScrollbarWidth();
> 
> This doesn't work with iframes, because the scrollbar width for an iframe comes from the iframe's RenderView's FrameView (which is a ScrollableArea)
> 
> I need to do more investigation before landing this. It seems likely that the best way to fix iframes is in another part of WebKit (not here)

Yeah, the fix for iframes is definitely somewhere else because position: static elements are also broken with iframes. iframes already do some coordinate conversion; I'll need to modify that instead of this place.

[Myles C. Maxfield]

Created attachment 274641 [details]
Fixes iframes

[Darin Adler]

Comment on attachment 274641 [details]
Fixes iframes

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2679
>      if (!m_dirtyRects.size())
>          return;

Unnecessary optimization; unlikely that this makes anything faster.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2682
> -    for (size_t i = 0; i < m_dirtyRects.size(); ++i)
> -        m_layer->setNeedsDisplayInRect(m_dirtyRects[i]);
> +    for (auto& dirtyRect : m_dirtyRects)
> +        m_layer->setNeedsDisplayInRect(dirtyRect);

Nice to convert this but seems unrelated to the other changes in this patch.

> Source/WebCore/rendering/RenderBlock.cpp:1067
> +        if (positionedObject->style().position() != FixedPosition)
> +            addOverflowFromChild(positionedObject, LayoutSize(positionedObject->x(), positionedObject->y()));

I’d be tempted to write the LayoutSize construction without naming the type, if it works:

    addOverflowFromChild(positionedObject, { positionedObject->x(), positionedObject->y() });

> Source/WebCore/rendering/RenderBox.cpp:3525
> +    if (is<RenderBox>(*containerBlock)) {

Unclear to me from the code above that containerBlock is guaranteed to be non-null. If it is, it would be more modern code style for us to use a reference for it the whole time.

> Source/WebCore/rendering/RenderBox.cpp:3526
> +        const RenderBox& containingBlock = downcast<RenderBox>(*containerBlock);

When we name the type in downcast, we have been using auto to avoid mentioning the type twice on one line:

    auto& containingBlock = downcast<RenderBox>(*containerBlock);

I think it’s too clever to subtly change the word “container” to “containing” when the real change is a cast to the type RenderBox. I would call this containingBox or containingBlockAsBox or whatever.

> Source/WebCore/rendering/RenderView.cpp:641
> +        FrameView& frameView = this->frameView();
> +        if (frameView.verticalScrollbarIsOnLeft() && frameView.verticalScrollbar())
> +            adjustedRect.move(LayoutSize(frameView.verticalScrollbar()->occupiedWidth(), 0));

I think the need for this code is quite non-obvious. I’m not saying it’s wrong, but simply that almost seems to be talking about a different subject than the rest of the code here. Perhaps there is some comment to write or other way to make clear that this code is needed here, and not in other similar code paths elsewhere.

[Myles C. Maxfield]

Committed r198560: <http://trac.webkit.org/changeset/198560>

[Myles C. Maxfield]

Committed r198564: <http://trac.webkit.org/changeset/198564>