Bug 161611 - Support ScrollIntoViewOptions for Element.scrollIntoView
Summary: Support ScrollIntoViewOptions for Element.scrollIntoView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: cathiechen
URL: http://w3c-test.org/css/cssom-view/sc...
Keywords: InRadar, WebExposed
: 167336 196787 (view as bug list)
Depends on: 199451 189258 199509
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-05 11:42 PDT by Simon Fraser (smfr)
Modified: 2019-07-09 03:03 PDT (History)
21 users (show)

See Also:


Attachments
Patch (12.77 KB, patch)
2019-06-26 07:19 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.53 MB, application/zip)
2019-06-26 08:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (3.05 MB, application/zip)
2019-06-26 08:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (3.23 MB, application/zip)
2019-06-26 09:19 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (3.34 MB, application/zip)
2019-06-26 10:55 PDT, Build Bot
no flags Details
Patch (13.54 KB, patch)
2019-07-01 10:31 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (13.58 KB, patch)
2019-07-02 02:35 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (19.40 KB, patch)
2019-07-07 08:14 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (19.47 KB, patch)
2019-07-08 09:10 PDT, cathiechen
no flags Details | Formatted Diff | Diff
Patch (19.52 KB, patch)
2019-07-09 00:02 PDT, cathiechen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Simon Fraser (smfr) 2017-01-25 07:23:04 PST
*** Bug 167336 has been marked as a duplicate of this bug. ***
Comment 2 Justin Mecham 2017-01-25 07:35:51 PST
(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.
Comment 3 Simon Pieters 2017-09-12 04:10:42 PDT
Also see https://github.com/w3c/csswg-drafts/pull/1505
Comment 4 jonjohnjohnson 2018-04-09 17:56:53 PDT
Just want to ensure general history traversal is affected with webkits implementation
https://github.com/w3c/csswg-drafts/issues/2454
Comment 5 Frédéric Wang (:fredw) 2018-07-26 06:49:15 PDT
(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.
Comment 6 Frédéric Wang (:fredw) 2018-09-04 00:24:39 PDT
(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.
Comment 7 Frédéric Wang (:fredw) 2018-09-12 07:23:36 PDT
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.
Comment 8 Frédéric Wang (:fredw) 2018-09-17 02:22:41 PDT
We should import WPT test css/cssom-view/scrollIntoView-vertical-rl-writing-mode.html
Comment 9 Frédéric Wang (:fredw) 2019-04-12 17:52:42 PDT
*** Bug 196787 has been marked as a duplicate of this bug. ***
Comment 10 cathiechen 2019-06-26 07:19:38 PDT
Created attachment 372922 [details]
Patch
Comment 11 Build Bot 2019-06-26 08:31:11 PDT
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
Comment 12 Build Bot 2019-06-26 08:31:13 PDT
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
Comment 13 Frédéric Wang (:fredw) 2019-06-26 08:38:30 PDT
cchen: I'll take a look into it soon ;-)
Comment 14 Build Bot 2019-06-26 08:39:34 PDT
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
Comment 15 Build Bot 2019-06-26 08:39:36 PDT
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 16 Build Bot 2019-06-26 09:19:33 PDT
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
Comment 17 Build Bot 2019-06-26 09:19:35 PDT
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 18 Frédéric Wang (:fredw) 2019-06-26 09:44:06 PDT
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 19 Build Bot 2019-06-26 10:55:22 PDT
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
Comment 20 Build Bot 2019-06-26 10:55:24 PDT
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 21 cathiechen 2019-06-26 22:11:00 PDT
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 22 Frédéric Wang (:fredw) 2019-06-26 22:30:29 PDT
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 23 Frédéric Wang (:fredw) 2019-06-26 22:48:31 PDT
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...
Comment 24 jonjohnjohnson 2019-06-29 07:24:59 PDT
(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.
Comment 25 cathiechen 2019-07-01 10:31:34 PDT
Created attachment 373246 [details]
Patch
Comment 26 cathiechen 2019-07-02 02:35:05 PDT
Created attachment 373311 [details]
Patch
Comment 27 cathiechen 2019-07-02 02:40:55 PDT
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.
Comment 28 cathiechen 2019-07-02 02:48:58 PDT
(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.
Comment 29 Radar WebKit Bug Importer 2019-07-03 11:05:56 PDT
<rdar://problem/52598804>
Comment 30 cathiechen 2019-07-07 08:14:05 PDT
Created attachment 373597 [details]
Patch
Comment 31 cathiechen 2019-07-08 02:51:28 PDT
Comment on attachment 373597 [details]
Patch

Hi,

This patch is ready to be reviewed. Please take a look. Thanks:)
Comment 32 Frédéric Wang (:fredw) 2019-07-08 03:49:35 PDT
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 33 cathiechen 2019-07-08 09:04:46 PDT
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.
Comment 34 cathiechen 2019-07-08 09:10:07 PDT
Created attachment 373630 [details]
Patch
Comment 35 Frédéric Wang (:fredw) 2019-07-08 23:32:51 PDT
Comment on attachment 373630 [details]
Patch

LGTM as well.
Comment 36 cathiechen 2019-07-09 00:02:02 PDT
Created attachment 373700 [details]
Patch
Comment 37 WebKit Commit Bot 2019-07-09 03:03:25 PDT
Comment on attachment 373700 [details]
Patch

Clearing flags on attachment: 373700

Committed r247255: <https://trac.webkit.org/changeset/247255>
Comment 38 WebKit Commit Bot 2019-07-09 03:03:28 PDT
All reviewed patches have been landed.  Closing bug.