1789464 - Incorrect horizontal alignment via scrollIntoView for RTL direction

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, defect, P3)

Description

[Stanislav Goncharov [:thegoncharov]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:104.0) Gecko/20100101 Firefox/104.0

Steps to reproduce:

1. Create a container with direction: rtl , overflow: scroll and some children.
2. Try to scroll container to some child element by scrollIntoView with inline: "start" alignment

element.scrollIntoView({
      behavior: "smooth",
      inline: "start",
      block: "nearest",
    })

Actual results:

Element will be aligned to the left side of container (like we get for direction: ltr)

Expected results:

Element should be aligned to the right side of container as for direction: rtl the flow must be inverted

Similar should be applied for `inline: "end"

Comment 1

[Stanislav Goncharov [:thegoncharov]]

Here's some demo https://wicgpc.csb.app/ (Press Scroll both lists).

Maybe it can be resolved by inverting alignment in dom/base/Element when

GetPrimaryFrame()->GetWritingMode().GetInlineDir() == WritingMode::InlineDir::eInlineRTL

?

This also should resolve WPT-test cssom-view/scrollIntoView-horizontal-tb-writing-mode-and-rtl-direction.html (which is ignored now)

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Yes, this doesn't take writing-modes into account at all: https://searchfox.org/mozilla-central/rev/9f8e74292115b4fbbb41698bfe9a7d8cc12b31cf/dom/base/Element.cpp#780-814

Comment 3

[Stanislav Goncharov [:thegoncharov]]

Attachment: Bug 1789464 - Incorrect horizontal alignment via scrollIntoView for RTL direction. r=emilio

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a live testcase, loosely based on comment 0.

STR with this testcase:

1. Click the button

EXPECTED RESULTS:
Blue box should be at the right side of the container (that's the inline-start side).

ACTUAL RESULTS::
Blue box ends up at the left side of the container.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (with `writing-mode: vertical-lr`)

Here's a testcase with default direction and with writing-mode: vertical-lr.

For this one, after the button is clicked, EXPECTED RESULTS are for the blue box to end up at bottom-left corner, but we scroll to place it at the top-right corner instead (likely because we don't handle the writing-mode properly).

This testcase uses these params for scrollIntoView:

 inline: "end",
 block: "start",

...and given those params plus our vertical-lr writing-mode: the bottom edge is the "end" edge of the inline axis; and the left edge is the "start" edge of the block axis, so that's why the expected results are what they are. It seems like we're probably just mistakenly interpreting inline as the horizontal axis and block as the vertical axis.

Comment 7

[Daniel Holbert [:dholbert]]

Ideally a complete fix here will address all of the test failures in bug 1950252, plus some more -- in particular, these .ini files should all be removable once we support arbitrary direction/writing-mode on the container here:

https://searchfox.org/mozilla-central/source/testing/web-platform/meta/css/cssom-view/scrollIntoView-horizontal-tb-writing-mode-and-rtl-direction.html.ini
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/css/cssom-view/scrollIntoView-sideways-lr-writing-mode-and-rtl-direction.html.ini
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/css/cssom-view/scrollIntoView-sideways-lr-writing-mode.html.ini
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/css/cssom-view/scrollIntoView-sideways-rl-writing-mode-and-rtl-direction.html.ini
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/css/cssom-view/scrollIntoView-sideways-rl-writing-mode.html.ini
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/css/cssom-view/scrollIntoView-vertical-lr-writing-mode-and-rtl-direction.html.ini
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/css/cssom-view/scrollIntoView-vertical-lr-writing-mode.html.ini
https://searchfox.org/mozilla-central/source/testing/web-platform/meta/css/cssom-view/scrollIntoView-vertical-rl-writing-mode.html.ini

(and also https://searchfox.org/mozilla-central/source/testing/web-platform/meta/css/cssom-view/scrollIntoView-horizontal-tb-writing-mode.html.ini which is a probably-stale "allowed to either timeout or pass" annotation for android+fission)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (like 2 but now the writing-mode is specified on the scrolled-into-view-thing instead of on the scroller)

Here's a testcase just like testcase 2 but where I moved the writing-mode to the thing that I'm calling scrollIntoView on (rather than specifying it on the scroller).

The expected results are the same. The point is that we should interpret start/end in terms of that element's logical axes, rather than the scroller's logical axes.

Comment 9

[Jonathan Kew [:jfkthame]]

Hey Daniel - I just noticed you assigned this to yourself, but in the meantime I have a patch locally that seems to fix the behavior in all these testcases. I'll go ahead and post what I have, but feel free to replace it with an alternative if you've got something better in the works. (Sorry about potentially getting in your way!)

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1789464 - Resolve logical axes in PresShell::ScrollFrameIntoView according to the target's writing mode. r=dholbert

Comment 11

[Daniel Holbert [:dholbert]]

No worries! Thanks for the patch.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68a1543cecb8
Resolve logical axes in PresShell::ScrollFrameIntoView according to the target's writing mode. r=emilio

Comment 13

[Cristina Horotan [:chorotan]]

Backed out for causing WR failures

Backout link

Push with failures

Failure log

Comment 14

[Daniel Holbert [:dholbert]]

Haven't looked closely, but the newly-failing tests are in /css/css-view-transitions/ -- it's possible (though worth confirming) that the failures are to-be-expected, and this bug happened to be papering over them somehow (and the patch is un-papering-over them).

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Probably due to my patches that were backed out instead, I'd reland

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41354ef260f0
Resolve logical axes in PresShell::ScrollFrameIntoView according to the target's writing mode. r=emilio,TYLin

Comment 17

[Jonathan Kew [:jfkthame]]

Thanks; yeah, I saw those failures on a try push, but concluded they were related to the recent view-transitions patches that were included, rather than this one.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4e799bcf05a
Annotate some view-transitions tests as failing.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

The spec text

2. Let scrolling box edge A be the beginning edge in the block flow direction of scrolling box, and let element edge A be target bounding border box’s edge on the same physical side as that of scrolling box edge A.

looks to me that the writing mode is for the scroll container, not for the target element. Am I mis-understanding the spec?

Comment 20

[Daniel Holbert [:dholbert]]

I agree that that's what it looks like from the spec, yeah. That disagrees with what I wrote in comment 8.

My notes in comment 8 (using the writing-mode of the target element) are based on what chrome and WebKit actually do.

I think I prefer the Chrome/WebKit behavior to what the spec says, I think... If we use the writing-mode of the scroller (really of each scroller), then scrollIntoView will do wildly different things for different scroll container ancestors.

Consider a case with nested scrollers, with one being horizontal-tb and another being vertical-lr, and then we call scrollIntoView on some element inside the innermost scroller with inline: "end"', block: "center".

• If we use the target-element's own writing-mode (whatever that is), we'll end-align the element in one axis and center it in the other axis, with those axes being consistent between the two nested scrollers.
• If we use the scrollers' writing-modes (which disagree with each other), then the outer scroller will center the element in the vertical axis and right-align it in the horizontal axis; and the inner scroller will center the element in the horizontal axis and bottom-align it in the vertical axis; and this probably results in it being placed at a confusing position/alignment.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

FWIW, though I don't have strong opinions, scroll-snap-align property is basically to resolve with the scroll container's writing mode, but there's an exception;

Start and end alignments are resolved with respect to the writing mode of the snap container unless the scroll snap area is larger than the snapport, in which case they are resolved with respect to the writing mode of the box itself. (This allows items in a container to have consistent snap alignment in general, while ensuring that start always aligns the item to allow reading its contents from the beginning.)

Comment 22

[Daniel Holbert [:dholbert]]

I agree it'd be nice to have these consistent, but also: note that scroll-snap-align is a bit simpler because there's only one scroll container that's honoring the snapping there, I think -- so specifying e.g. scroll-snap-align:end on an element wouldn't simultaneously cause us to snap to 3 different edges for 3 different nested scroll-container-ancestors (as it would for scrollIntoView with 3 nested scroll-container ancestors of differing writing-modes).

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: testcase 4 (with nested scrollers of differing writing-modes)

Here's a testcase along the lines of my "Consider a case ..." scenario in comment 20.

The Chrome/Safari behavior (which I think we'll now match?) produces reasonably intuitive results, with inline: "center", block: "start", giving us a box that's consistently aligned at the far side in one axis and centered in the other axis.

The spec behavior would instead have us do the following, I think:

• Scroll the inner scroll container (with writing-mode: vertical-rl) such that the blue box is right-aligned and vertically centered.
• Scroll the outer scroll container (with default writing-mode: horizontal-tb) such that the blue box is top-aligned and horizontally centered.
  ...and for me at least, that feels a bit weirder.

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: screenshot showing current Chrome/WebKit behavior with testcase 4 (respecting writing-mode of blue box, the scrollIntoView target)

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: screenshot of spec behavior with testcase 4 (respecting writing-mode of each scroller separately)

Comment 26

[Daniel Holbert [:dholbert]]

I've attached screenshots showing how the two different behaviors look for testcase 4, just for illustration purposes. (I manually scrolled to the expected positions in the second screenshot to represent what I think the spec technically calls for.)

I'm curious if jfkthame has opinions here; if we're largely in agreement (or not objecting) to aligning on the WebKit/Chromium behavior, we should probably file a spec bug to make it reflect reality (and/or to give the spec editors a chance to tell us why everyone's wrong and needs to change :) ).

Comment 27

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/41354ef260f0
https://hg.mozilla.org/mozilla-central/rev/a4e799bcf05a

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment: testcase 4: Nightly (with patch) / Chrome Canary / Safari Tech Preview

Interestingly, I actually see somewhat different results in Chrome vs Safari, as seen in the screenshot here (Chrome in the middle, Safari on the right): although the ultimate position of the blue box is similar, its position within its enclosing scroller is at the bottom instead of the middle (and the outer scroller is adjusted to compensate for that).

Firefox's behavior, once the patch here is included, is the same as Chrome (except for the question of which side of the inner scroller its vertical scrollbar appears: in my screenshot, it's out of view to the left of the outer scroller).

Anyhow, at this point we all end up with the target element in basically the same place, and I'd be fine with aligning on this (and adjusting the spec accordingly); it seems preferable to what the spec describes for nested scrollers with mixed directionalities.

Comment 29

[Emilio Cobos Álvarez (:emilio)]

+1, unlike scroll snap which is a property of the scroller, scrollIntoView takes the target, it'd be weird if the directions were contextual on the writing mode of the scroller(s).

Comment 31

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Kew [:jfkthame] from comment #28)

Interestingly, I actually see somewhat different results in Chrome vs Safari, as seen in the screenshot here (Chrome in the middle, Safari on the right): although the ultimate position of the blue box is similar, its position within its enclosing scroller is at the bottom instead of the middle (and the outer scroller is adjusted to compensate for that).

Ah, I see that too. I'm pretty sure that's just because WebKit doesn't generate any vertical scrollable overflow after the blue element for some reason, inside that inner scroller -- note that the inner scrollbar is scrolled to the very end there, so they literally can't scroll any further to center the blue element.

That's an interesting difference but it's unrelated to the issues discussed here. I suspect it's related to the reason that WebKit fails https://wpt.fyi/results/css/css-writing-modes/wm-propagation-body-044.html (where they place some vertical-writing-mode content to the right of the previous sibling, instead of to the bottom of it) -- I'll bet it's a version of an already-known (or already-exposed-via-WPTs) WebKit bug, so I'm not going to poke too much or file a new webkit bug on it.

Comment 32

[Daniel Holbert [:dholbert]]

I filed https://github.com/w3c/csswg-drafts/issues/11796 to suggest that we change the spec to match reality.

Comment 33

[Daniel Holbert [:dholbert]]

ah, the WebKit difference in testcase 4 (comment 30-31) is just that WebKit doesn't treat margins as generating scrollable overflow at the inline-end edge of a scroller. We didn't either until we fixed bug 1768921. So that's just WebKit's version of bug 1768921.

Here's a testcase to demonstrate that difference:

data:text/html,<!DOCTYPE html>
<div style="overflow:scroll; border: 5px solid black; height: 200px; width: 200px">
<div style="margin-inline-end: 600px; margin-block-end: 600px">

Chrome and Firefox (with bug 1768921's fix) get a horizontal scrollbar but WebKit (and Firefox before bug 1768921's fix) do not. All browsers show a vertical scrollbar.

I'll post another testcase that's not reliant on that inline-end margin behavior, in case it's useful in the spec discussion.

Comment 34

[Daniel Holbert [:dholbert]]

Attachment: testcase 5: better version of testcase 4

Here's a better version of testcase 4 that doesn't depend on the new behavior from https://github.com/w3c/csswg-drafts/issues/8660 (which I think WebKit hasn't implemented yet).

This one produces consistent results in Nightly (with the patch that landed here), Chrome, and Safari.