Bug 161611

*** Bug 167336 has been marked as a duplicate of this bug. ***

(In reply to comment #1) > *** Bug 167336 has been marked as a duplicate of this bug. *** I'm not certain this is a duplicate, although implementation of this bug (161611) would also allow the `scroll-behavior` CSS to be implemented. My apologies for filing a separate issue if this was understood.

Also see https://github.com/w3c/csswg-drafts/pull/1505

Just want to ensure general history traversal is affected with webkits implementation https://github.com/w3c/csswg-drafts/issues/2454

(In reply to Justin Mecham from comment #2) > (In reply to comment #1) > > *** Bug 167336 has been marked as a duplicate of this bug. *** > > I'm not certain this is a duplicate, although implementation of this bug > (161611) would also allow the `scroll-behavior` CSS to be implemented. My > apologies for filing a separate issue if this was understood. I personally don't think it was a duplicate. The `scroll-behavior` and ScrollBehavior could probably be implemented independently but it makes sense to do them together, given that the hard part is really doing smooth scrolling, not parsing new CSS properties or DOM parameter. I opened a separate bug 188043 for that. ScrollIntoViewOptions has some ScrollLogicalPosition parameters to specify alignments, which I think can be implemented here independently of the scroll behavior feature (and vice-versa), similarly to what was done in bug 161610 for ScrollToOptions.

(In reply to Frédéric Wang (:fredw) from comment #5) > ScrollIntoViewOptions has some ScrollLogicalPosition parameters to specify > alignments, which I think can be implemented here independently of the > scroll behavior feature (and vice-versa), similarly to what was done in bug > 161610 for ScrollToOptions. So thinking about it, I think it will be more convenient to introduce ScrollIntoViewOptions before implementing scroll-behavior. I think this can be a first basic implementation (that maybe does not match exactly CSSOM View): just map block/inline parameters into the proper parameters of scrollRectToVisible, with "nearest" interpreted as ScrollAlignment::alignToEdgeIfNeeded and perhaps ignoring directionality/orientation for now. This would at least make thing compatible with the current one-parameter implementation and avoid losing the alignment features when one passes ScrollBehavior.

Random thought for whoever is going to work on this: Converting logical to non-logical alignments as currently done in Element::scrollIntoView won't probably work in the general case (i.e. if we have nested scrollable elements with different orientation/direction). Instead my guess is that ScrollRectToVisibleOptions should contain the logicial alignments and RenderLayer::scrollRectToVisible should interpret them for each scrollable ancestor according to the applied style.

We should import WPT test css/cssom-view/scrollIntoView-vertical-rl-writing-mode.html

*** Bug 196787 has been marked as a duplicate of this bug. ***

Created attachment 372922 [details] Patch

Comment on attachment 372922 [details] Patch Attachment 372922 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12581256 New failing tests: fast/events/scroll-to-anchor-vertical-writing-mode.html fast/events/scroll-to-anchor-vertical-lr-writing-mode.html fast/events/scroll-to-anchor-vertical-writing-mode-contained.html

Created attachment 372925 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6

cchen: I'll take a look into it soon ;-)

Comment on attachment 372922 [details] Patch Attachment 372922 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12581282 New failing tests: fast/events/scroll-to-anchor-vertical-writing-mode.html fast/events/scroll-to-anchor-vertical-lr-writing-mode.html fast/events/scroll-to-anchor-vertical-writing-mode-contained.html

Created attachment 372926 [details] Archive of layout-test-results from ews107 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 372922 [details] Patch Attachment 372922 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12581358 New failing tests: fast/events/scroll-to-anchor-vertical-writing-mode-contained.html

Created attachment 372929 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.5

Comment on attachment 372922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372922&action=review Thanks, I'll need more time to understand this patch but here are a few comments inline. > Source/WebCore/dom/Element.cpp:733 > + } https://drafts.csswg.org/cssom-view/#scroll-an-element-into-view relies on https://drafts.csswg.org/css-writing-modes-4/#inline-base-direction s So you also need to check whether the CSS direction property is RTL or LTR to decide whether you return right/bottom/left/top. > LayoutTests/imported/w3c/ChangeLog:12 > + I think you should upload a PR to WPT, probably cc'ing the original author/reviewer.

Comment on attachment 372922 [details] Patch Attachment 372922 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12581984 New failing tests: fast/events/scroll-to-anchor-vertical-writing-mode.html fast/events/scroll-to-anchor-vertical-lr-writing-mode.html fast/events/scroll-to-anchor-vertical-writing-mode-contained.html

Created attachment 372935 [details] Archive of layout-test-results from ews116 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-highsierra Platform: Mac OS X 10.13.6

Comment on attachment 372922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372922&action=review Hi Fred, Thanks for the review! I added some explanation for this patch. Hope to make it clear. > Source/WebCore/dom/Element.cpp:704 > +inline ScrollAlignment toScrollAlignmentForInlineDirection(Optional<ScrollLogicalPosition> position, WritingMode writingMode) This function decide which physical side of the inline direction alignment should be. E.g. writing-mode: vertical-rl and scrollIntoViewOptions is inline: "start". The physical alignment side is top. > Source/WebCore/dom/Element.cpp:743 > +inline ScrollAlignment toScrollAlignmentForBlockDirection(Optional<ScrollLogicalPosition> position, WritingMode writingMode) This function the block direction. E.g. writing-mode: vertical-rl and scrollIntoViewOptions is block: "start". The physical alignment side is right. > Source/WebCore/rendering/RenderLayer.cpp:2803 > + auto physicalScrollY = isHorizontal ? scrollY : scrollX; In order to compute the X and Y coordinate(physical), we should switch ScrollX/ScrollY to physical here. E.g. writing-mode: vertical-rl and scrollIntoViewOptions {block: "start", inline: "start"} scrollX: AlignTop which is effective to Y coordinate. scrollY: AlignLeft which is effective to X coordinate.

Comment on attachment 372922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372922&action=review > Source/WebCore/ChangeLog:18 > + I would be nice to complete the changelogs with more per-file details. > Source/WebCore/rendering/RenderLayer.cpp:2798 > + // Given the behavior and WritingMode, compute the X and Y coordinate. scrollX and scrollY behaviorS X and Y coordinateS >> Source/WebCore/rendering/RenderLayer.cpp:2803 >> + auto physicalScrollY = isHorizontal ? scrollY : scrollX; > > In order to compute the X and Y coordinate(physical), we should switch ScrollX/ScrollY to physical here. > > E.g. writing-mode: vertical-rl and scrollIntoViewOptions {block: "start", inline: "start"} > scrollX: AlignTop which is effective to Y coordinate. > scrollY: AlignLeft which is effective to X coordinate. This switches the coordinates depending on writing mode, but only the Element::scrollIntoView caller has been logicalized to switch scrollX/scrollY accordingly. That might explain why some tests fail. > Source/WebCore/rendering/RenderLayer.cpp:2810 > + x = exposeRect.maxX() - visibleRect.width(); nit: I think you don't need to switch the order of AlignRight / AlignCenter conditionals. That would keep things consistent with the Y case below. > Source/WebCore/rendering/RenderLayer.cpp:2812 > + x = exposeRect.x(); Probably we should assert that physicalScrollX is not a vertical alignment... > Source/WebCore/rendering/RenderLayer.cpp:2821 > y = exposeRect.y(); And that physicalScrollY is not a horizontal alignment...

Comment on attachment 372922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372922&action=review >> Source/WebCore/dom/Element.cpp:733 >> + } > > https://drafts.csswg.org/cssom-view/#scroll-an-element-into-view > relies on https://drafts.csswg.org/css-writing-modes-4/#inline-base-direction > s > > So you also need to check whether the CSS direction property is RTL or LTR to decide whether you return right/bottom/left/top. It seems WPT does not have any test for this. Can you please add one? > LayoutTests/imported/w3c/web-platform-tests/css/cssom-view/scrollIntoView-vertical-rl-writing-mode.html:70 > // to match the semantics of scrollLeft. Mmh, this looks a hack to workaround browser inconsistency. Probably you need to discuss with the test/spec author...

(In reply to jonjohnjohnson from comment #4) > Just want to ensure general history traversal is affected with webkits implementation > https://github.com/w3c/csswg-drafts/issues/2454 Highlighting this as I don't see any responses in the thread, since it was overlooked on release in both blink and gecko.

Created attachment 373246 [details] Patch

Created attachment 373311 [details] Patch

Comment on attachment 372922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372922&action=review >> Source/WebCore/ChangeLog:18 >> + > > I would be nice to complete the changelogs with more per-file details. Done >>> Source/WebCore/dom/Element.cpp:733 >>> + } >> >> https://drafts.csswg.org/cssom-view/#scroll-an-element-into-view >> relies on https://drafts.csswg.org/css-writing-modes-4/#inline-base-direction >> s >> >> So you also need to check whether the CSS direction property is RTL or LTR to decide whether you return right/bottom/left/top. > > It seems WPT does not have any test for this. Can you please add one? Done >>> Source/WebCore/rendering/RenderLayer.cpp:2803 >>> + auto physicalScrollY = isHorizontal ? scrollY : scrollX; >> >> In order to compute the X and Y coordinate(physical), we should switch ScrollX/ScrollY to physical here. >> >> E.g. writing-mode: vertical-rl and scrollIntoViewOptions {block: "start", inline: "start"} >> scrollX: AlignTop which is effective to Y coordinate. >> scrollY: AlignLeft which is effective to X coordinate. > > This switches the coordinates depending on writing mode, but only the Element::scrollIntoView caller has been logicalized to switch scrollX/scrollY accordingly. That might explain why some tests fail. Yes, that's reason. I think I could switch scrollX/scrollY in Element::scrollIntoView. So the change in this file could be reverted. >> LayoutTests/imported/w3c/ChangeLog:12 >> + > > I think you should upload a PR to WPT, probably cc'ing the original author/reviewer. Done >> LayoutTests/imported/w3c/web-platform-tests/css/cssom-view/scrollIntoView-vertical-rl-writing-mode.html:70 >> // to match the semantics of scrollLeft. > > Mmh, this looks a hack to workaround browser inconsistency. Probably you need to discuss with the test/spec author... Yes, there's inconsistency. I'll try add comment in the github issue.

(In reply to jonjohnjohnson from comment #24) > (In reply to jonjohnjohnson from comment #4) > > Just want to ensure general history traversal is affected with webkits implementation > > https://github.com/w3c/csswg-drafts/issues/2454 > > Highlighting this as I don't see any responses in the thread, since it was > overlooked on release in both blink and gecko. Thanks for the reply:) Sorry, this patch fixes the alignment issue of ScrollIntoViewOptions, not the behavior.

<rdar://problem/52598804>

Created attachment 373597 [details] Patch

Comment on attachment 373597 [details] Patch Hi, This patch is ready to be reviewed. Please take a look. Thanks:)

Comment on attachment 373597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373597&action=review > Source/WebCore/ChangeLog:13 > + If direction: rtl and writing-mode: horizontal-tb, the scroll bar will be on the left side. The visible rect Maybe say "WebKit puts the scrollbar on the left side" since that's not defined by the spec. > Source/WebCore/dom/Element.cpp:713 > + return ScrollAlignment::alignRightAlways; You may use the syntax return isLTR ? ScrollAlignment::alignLeftAlways : ScrollAlignment::alignRightAlways; > Source/WebCore/dom/Element.cpp:824 > + renderer()->scrollRectToVisible(absoluteBounds, insideFixed, { SelectionRevealMode::Reveal, alignY, alignX, ShouldAllowCrossOriginScrolling::No }); Maybe create a ScrollRectToVisibleOptions initialized according to isHorizontalWritingMode(), so that you need only one call to scrollRectToVisible? > Source/WebCore/rendering/RenderLayer.cpp:2629 > + } Is it actually a fix for bug 199451? If so, I would do that first in a separate patch attached to bug 199451.

Comment on attachment 373597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373597&action=review Hi Fred, Thanks for the review:) >> Source/WebCore/ChangeLog:13 >> + If direction: rtl and writing-mode: horizontal-tb, the scroll bar will be on the left side. The visible rect > > Maybe say "WebKit puts the scrollbar on the left side" since that's not defined by the spec. Done. >> Source/WebCore/dom/Element.cpp:713 >> + return ScrollAlignment::alignRightAlways; > > You may use the syntax > > return isLTR ? ScrollAlignment::alignLeftAlways : ScrollAlignment::alignRightAlways; Done >> Source/WebCore/dom/Element.cpp:824 >> + renderer()->scrollRectToVisible(absoluteBounds, insideFixed, { SelectionRevealMode::Reveal, alignY, alignX, ShouldAllowCrossOriginScrolling::No }); > > Maybe create a ScrollRectToVisibleOptions initialized according to isHorizontalWritingMode(), so that you need only one call to scrollRectToVisible? Done >> Source/WebCore/rendering/RenderLayer.cpp:2629 >> + } > > Is it actually a fix for bug 199451? If so, I would do that first in a separate patch attached to bug 199451. Nope, the alignment didn't attach to the edge if there's left vertical scroll bar(direction: rtl; writing-mode: horizontal-tb;). The according test case is scrollIntoView-horizontal-tb-writing-mode-and-rtl-direction.html Sorry, the comment was not clear. More comments added to explain this.

Created attachment 373630 [details] Patch

Comment on attachment 373630 [details] Patch LGTM as well.

Created attachment 373700 [details] Patch

Comment on attachment 373700 [details] Patch Clearing flags on attachment: 373700 Committed r247255: <https://trac.webkit.org/changeset/247255>

All reviewed patches have been landed. Closing bug.