Scrolling to anchor tags does nothing in vertical-rl writing mode

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Scrolling to anchor tags does nothing in vertical-rl writing mode

[Myles C. Maxfield]

Created attachment 240047 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/15957180>

[Build Bot]

Created attachment 240393 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Dave Hyatt]

Comment on attachment 240047 [details]
Patch

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

r-

> Source/WebCore/dom/ContainerNode.cpp:929
> +    FloatPoint orientedUpperLeft(std::min(upperLeft.x(), lowerRight.x()), std::min(upperLeft.y(), lowerRight.y()));
> +    FloatPoint orientedLowerRight(std::max(upperLeft.x(), lowerRight.x()), std::max(upperLeft.y(), lowerRight.y()));
>  
> -    return enclosingLayoutRect(FloatRect(upperLeft, lowerRight.expandedTo(upperLeft) - upperLeft));
> +    return enclosingLayoutRect(FloatRect(orientedUpperLeft, orientedLowerRight.expandedTo(orientedUpperLeft) - orientedUpperLeft));

I do not understand this change. Isn't this boundingBox() method a physical method? With terms like "upper left" I assume it is physical, but it looks like you're changing it to do something else.

> Source/WebCore/page/FrameView.cpp:2789
> -    anchorNode->renderer()->scrollRectToVisible(rect, ScrollAlignment::alignToEdgeIfNeeded, ScrollAlignment::alignTopAlways);
> +    if (anchorNode->renderer()->style().isHorizontalWritingMode())
> +        anchorNode->renderer()->scrollRectToVisible(rect, ScrollAlignment::alignToEdgeIfNeeded, ScrollAlignment::alignTopAlways);
> +    else if (anchorNode->renderer()->style().isFlippedBlocksWritingMode())
> +        anchorNode->renderer()->scrollRectToVisible(rect, ScrollAlignment::alignRightAlways, ScrollAlignment::alignToEdgeIfNeeded);
> +    else
> +        anchorNode->renderer()->scrollRectToVisible(rect, ScrollAlignment::alignLeftAlways, ScrollAlignment::alignToEdgeIfNeeded);

I don't think the anchor node's writing mode is what you want. Isn't this really about the writing mode of the scrolling container? I think you need some mixed writing mode tests where you have e.g., a vertical-lr scrollable container but a horizontal-tb anchor (and vice versa) in order to make sure you're doing this properly.

[Dave Hyatt]

I would probably just add a new constant like ScrollAlignment::AlignLogicalTopAlways and then put code in scrollRectToVisible that does the right thing based off the writing mode of the container that actually scrolls.

[Dave Hyatt]

Alternatively we could just say AlignTopAlways means "logical top" and not physical, but you'd have to look at how it's used elsewhere in the code.

[Myles C. Maxfield]

Comment on attachment 240047 [details]
Patch

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

>> Source/WebCore/dom/ContainerNode.cpp:929
>> +    return enclosingLayoutRect(FloatRect(orientedUpperLeft, orientedLowerRight.expandedTo(orientedUpperLeft) - orientedUpperLeft));
> 
> I do not understand this change. Isn't this boundingBox() method a physical method? With terms like "upper left" I assume it is physical, but it looks like you're changing it to do something else.

In vertical-rl, "getUpperLeftCorner" gives you the top right corner, and "getLowerRightCorner" gets you the lower left corner. Perhaps I should rename these functions...

[Myles C. Maxfield]

Created attachment 242606 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 242606 [details]
Patch

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

> Source/WebCore/dom/ContainerNode.cpp:792
>      while (o) {

Could use RenderIterator here, but that is probably something for another patch.

[Myles C. Maxfield]

Comment on attachment 242606 [details]
Patch

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

> Source/WebCore/dom/ContainerNode.cpp:-897
> -    return enclosingLayoutRect(FloatRect(upperLeft, lowerRight.expandedTo(upperLeft) - upperLeft));

I should put back this expandedTo() call.

[Simon Fraser (smfr)]

Comment on attachment 242606 [details]
Patch

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

> Source/WebCore/dom/ContainerNode.cpp:-784
> -    // What is this code really trying to do?

Ha I think I added this comment.

> Source/WebCore/dom/Node.h:382
> +    virtual bool getLeadingCornerInAbsolute(FloatPoint&) const;
> +    virtual bool getTrailingCornerInAbsolute(FloatPoint&) const;

I don't think it's appropriate to expose these on Node (or even ContainerNode). Callers should jump to renderers to compute these.

> Source/WebCore/page/FrameView.cpp:2779
> +static LayoutRect anchorRect(PassRefPtr<Node> anchorNode)

Feels like this should not be in FrameView.

> Source/WebCore/page/FrameView.cpp:2784
> +    bool foundLeading = anchorNode->getLeadingCornerInAbsolute(leading);
> +    bool foundTrailing = anchorNode->getTrailingCornerInAbsolute(trailing);

So you're getting the same renderer, twice, and calling absoluteBoundingBoxRect() twice, just to get leading/trailing points? Boo!

> Source/WebCore/page/FrameView.cpp:2795
> +    RenderObject* renderer = anchorNode->renderer();

Oh look, you get the renderer anyway!

[Myles C. Maxfield]

See https://bugs.webkit.org/show_bug.cgi?id=139333 for a related bug.

[Myles C. Maxfield]

Comment on attachment 240047 [details]
Patch

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

>> Source/WebCore/page/FrameView.cpp:2789
>> +        anchorNode->renderer()->scrollRectToVisible(rect, ScrollAlignment::alignLeftAlways, ScrollAlignment::alignToEdgeIfNeeded);
> 
> I don't think the anchor node's writing mode is what you want. Isn't this really about the writing mode of the scrolling container? I think you need some mixed writing mode tests where you have e.g., a vertical-lr scrollable container but a horizontal-tb anchor (and vice versa) in order to make sure you're doing this properly.

I actually disagree that I should inspect the scrolling container instead of the anchor node. It seems to me that, when scrolling to an anchor tag, the user's intent is to interact with (read) the anchor node. Given that the user has just demonstrated their intent to start reading the text in the anchor node, it seems to me that we should be using their intent as a reference for how to scroll, rather than inspecting the ancestor that happens to be doing the scrolling.

For example, if you have a ltr document with a chunk of vertical-rl in the middle of it, and the user clicks on an anchor link to a block inside the vertical-rl part, it seems natural to me that we should always align the right edge of the block with the right edge of the screen. That way, we have just set the viewport up for them to be able to read the block of text, from the right side to the left side. (This is the same logic we use for positioning the top of the viewport on the anchor when everything is ltr).

That being said, it's not immediately obvious what to do for ancestors of the scrolling container that are, themselves, scrolling containers. Maybe using their own writing mode makes sense there. What do you think?

[Dave Hyatt]

Sorry I meant containing block not scrolling container. My point is the node itself is not what should be used.

(1) Consider an inline-block anchor with vertical-rl but sitting on a horizontal-tb line. The jump should be about the containing block of this anchor and not about the anchor itself.

(2) Other examples include floating anchors (vertical-rl float but inside a horizontal-tb block), or positioned anchors (positioned via left/top in horizontal-tb but vertical-rl inside).

If you use the containing block, I'm fairly certain that will give you the correct behavior. If the scrolling container is horizontal-tb but the anchor is inside a vertical-rl containing block, I agree that vertical-rl alignment is what you want.

[Dave Hyatt]

Actually never mind. I think using the node is a good starting point. It's pretty difficult to figure out intent when the anchor is the writing mode boundary, but I think erring on the side of assuming that the user wants to read what is inside the anchor makes sense.

[Myles C. Maxfield]

Created attachment 242965 [details]
Patch

[Dave Hyatt]

Comment on attachment 242965 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 242965 [details]
Patch

Clearing flags on attachment: 242965

Committed r177050: <http://trac.webkit.org/changeset/177050>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.