Bug 205009 - Add support for scroll-behavior's css property and ScrollOptions parsing
Summary: Add support for scroll-behavior's css property and ScrollOptions parsing
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:
Keywords: InRadar
Depends on:
Blocks: 188043 204882
  Show dependency treegraph
 
Reported: 2019-12-09 06:59 PST by cathiechen
Modified: 2020-04-16 20:58 PDT (History)
20 users (show)

See Also:


Attachments
Patch (62.14 KB, patch)
2019-12-09 07:31 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (62.15 KB, patch)
2019-12-10 03:51 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (62.42 KB, patch)
2019-12-10 04:36 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (64.57 KB, patch)
2019-12-10 07:33 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (62.50 KB, patch)
2019-12-10 22:42 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (62.52 KB, patch)
2020-01-13 06:23 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (65.80 KB, patch)
2020-01-16 21:52 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (65.84 KB, patch)
2020-01-17 00:42 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (2.77 KB, patch)
2020-01-18 21:19 PST, Brady Eidson
ddkilzer: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cathiechen 2019-12-09 06:59:48 PST
Add support for scroll-behavior's css property and ScrollOptions parsing.
And add an experimental flag to guard.
Comment 1 cathiechen 2019-12-09 07:31:27 PST
Created attachment 385148 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2019-12-09 08:17:31 PST
Comment on attachment 385148 [details]
Patch

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

OK, this LGTM but I don't know if we should land this patch now + I'm the author of most of this part, so someone else should probably review.

> Source/WebCore/Sources.txt:1755
> +platform/ScrollAnimationSmooth.cpp

That change does not seem necessary for now.

> Source/WebCore/SourcesGTK.txt:-66
> -platform/ScrollAnimationSmooth.cpp

That one either.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:178
> +    unsigned useSmoothScrolling : 1; // ScrollBehavior

I wonder if this should be placed somewhere else to optimize bitfield alignment but I'm not really sure what's the recommendation in WebKit.

> LayoutTests/platform/mac-wk1/TestExpectations:748
> +webkit.org/b/191357 imported/w3c/web-platform-tests/css/cssom-view/inheritance.html [ Skip ]

Maybe skipping that one is not necessary, since it's just css parsing.
Comment 3 cathiechen 2019-12-10 03:45:48 PST
Comment on attachment 385148 [details]
Patch

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

Hi Fred,
Thanks for the review:)

>> Source/WebCore/Sources.txt:1755
>> +platform/ScrollAnimationSmooth.cpp
> 
> That change does not seem necessary for now.

Done, move this change to web scroll patch.

>> Source/WebCore/SourcesGTK.txt:-66
>> -platform/ScrollAnimationSmooth.cpp
> 
> That one either.

Ditto

>> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:178
>> +    unsigned useSmoothScrolling : 1; // ScrollBehavior
> 
> I wonder if this should be placed somewhere else to optimize bitfield alignment but I'm not really sure what's the recommendation in WebKit.

Done, put it behind textOverflow.

>> LayoutTests/platform/mac-wk1/TestExpectations:748
>> +webkit.org/b/191357 imported/w3c/web-platform-tests/css/cssom-view/inheritance.html [ Skip ]
> 
> Maybe skipping that one is not necessary, since it's just css parsing.

inheritance.html is passed on non-mac-wk1, but not on mac-wk1. 
So I guess, maybe we can skip it first and figure this out in the follow-up bug?
Comment 4 cathiechen 2019-12-10 03:51:12 PST
Created attachment 385247 [details]
Patch
Comment 5 cathiechen 2019-12-10 04:36:51 PST
Created attachment 385250 [details]
Patch
Comment 6 Frédéric Wang (:fredw) 2019-12-10 06:27:00 PST
Comment on attachment 385148 [details]
Patch

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

>>> LayoutTests/platform/mac-wk1/TestExpectations:748
>>> +webkit.org/b/191357 imported/w3c/web-platform-tests/css/cssom-view/inheritance.html [ Skip ]
>> 
>> Maybe skipping that one is not necessary, since it's just css parsing.
> 
> inheritance.html is passed on non-mac-wk1, but not on mac-wk1. 
> So I guess, maybe we can skip it first and figure this out in the follow-up bug?

I don't understand why this is failing on WK1 but in any case, I maybe the comment should be updated to better explain why.
Comment 7 cathiechen 2019-12-10 07:31:26 PST
Comment on attachment 385148 [details]
Patch

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

>>>> LayoutTests/platform/mac-wk1/TestExpectations:748
>>>> +webkit.org/b/191357 imported/w3c/web-platform-tests/css/cssom-view/inheritance.html [ Skip ]
>>> 
>>> Maybe skipping that one is not necessary, since it's just css parsing.
>> 
>> inheritance.html is passed on non-mac-wk1, but not on mac-wk1. 
>> So I guess, maybe we can skip it first and figure this out in the follow-up bug?
> 
> I don't understand why this is failing on WK1 but in any case, I maybe the comment should be updated to better explain why.

Done.
Comment 8 cathiechen 2019-12-10 07:33:45 PST
Created attachment 385258 [details]
Patch
Comment 9 cathiechen 2019-12-10 22:42:25 PST
Created attachment 385351 [details]
Patch
Comment 10 cathiechen 2020-01-13 06:23:12 PST
Created attachment 387520 [details]
Patch
Comment 11 cathiechen 2020-01-13 06:26:02 PST
Rebase the patch.
Comment 12 cathiechen 2020-01-16 21:52:36 PST
Created attachment 388010 [details]
Patch
Comment 13 cathiechen 2020-01-17 00:42:18 PST
Created attachment 388018 [details]
Patch
Comment 14 cathiechen 2020-01-17 01:11:57 PST
Rebase the patch.

This patch has presented for a while. We plan to land it tomorrow.
Please leave a msg if there's any concern. Thanks:)
Comment 15 WebKit Commit Bot 2020-01-17 21:49:31 PST
Comment on attachment 388018 [details]
Patch

Clearing flags on attachment: 388018

Committed r254790: <https://trac.webkit.org/changeset/254790>
Comment 16 WebKit Commit Bot 2020-01-17 21:49:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2020-01-17 21:50:20 PST
<rdar://problem/58705707>
Comment 18 Brady Eidson 2020-01-18 21:19:27 PST
You missed updating the DerivedSources xcfilelists
Comment 19 Brady Eidson 2020-01-18 21:19:41 PST
Reopening to attach new patch.
Comment 20 Brady Eidson 2020-01-18 21:19:42 PST
Created attachment 388169 [details]
Patch
Comment 21 David Kilzer (:ddkilzer) 2020-01-18 22:04:12 PST
(In reply to Brady Eidson from comment #20)
> Created attachment 388169 [details]
> Patch

This commit will fail because I just committed the same build fix in r254802:

<https://trac.webkit.org/r254802>

Sorry about that.
Comment 22 Brady Eidson 2020-01-18 22:04:57 PST
(In reply to David Kilzer (:ddkilzer) from comment #21)
> (In reply to Brady Eidson from comment #20)
> > Created attachment 388169 [details]
> > Patch
> 
> This commit will fail because I just committed the same build fix in r254802:
> 
> <https://trac.webkit.org/r254802>
> 
> Sorry about that.

Way to cowboy it :)
Comment 23 David Kilzer (:ddkilzer) 2020-01-18 22:05:55 PST
(In reply to Brady Eidson from comment #22)
> (In reply to David Kilzer (:ddkilzer) from comment #21)
> > (In reply to Brady Eidson from comment #20)
> > > Created attachment 388169 [details]
> > > Patch
> > 
> > This commit will fail because I just committed the same build fix in r254802:
> > 
> > <https://trac.webkit.org/r254802>
> > 
> > Sorry about that.
> 
> Way to cowboy it :)

Honestly, I didn't think anyone else would be trying to build WebKit at this time of the night. :)
Comment 24 cathiechen 2020-01-18 23:58:38 PST
Hi,
Sorry for the troubles and thanks for fixing this:)