Bug 226399

Summary:

background-attachment:fixed inside position:fixed shouldn't cause slow scrolling

Product:

WebKit

Reporter:

Cameron McCormack (:heycam) <heycam>

Component:

Scrolling

Assignee:

Cameron McCormack (:heycam) <heycam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

changseok, cmarcelo, esprehn+autocc, ews-watchlist, fred.wang, glenn, jamesr, kondapallykalyan, luiz, pdr, simon.fraser, tonikitoo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

226453

Bug Blocks:

Attachments:

Description	Flags
Squashed patch with dependencies for EWS	none
Squashed patch with dependencies for EWS	none
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none

Description Cameron McCormack (:heycam) 2021-05-28 15:09:12 PDT

Background images that are background-attachment:fixed don't need to cause slow scrolling when inside position:fixed elements, since they won't move.

Comment 1 Cameron McCormack (:heycam) 2021-05-28 15:09:47 PDT

<rdar://72830073>

Comment 2 Cameron McCormack (:heycam) 2021-05-30 22:36:54 PDT

Created attachment 430175 [details]
Squashed patch with dependencies for EWS

Comment 3 Cameron McCormack (:heycam) 2021-05-31 23:08:47 PDT

Created attachment 430232 [details]
Squashed patch with dependencies for EWS

Comment 4 Cameron McCormack (:heycam) 2021-06-01 15:21:03 PDT

Created attachment 430297 [details]
Patch

Comment 5 Cameron McCormack (:heycam) 2021-06-01 16:05:10 PDT

Comment on attachment 430297 [details]
Patch

Let's fix up this build bustage from dropping the bug 226453 patch.

Comment 6 Cameron McCormack (:heycam) 2021-06-01 17:30:58 PDT

Created attachment 430309 [details]
Patch

Comment 7 Simon Fraser (smfr) 2021-06-01 21:01:13 PDT

Comment on attachment 430309 [details]
Patch

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

Seems OK, but it's worrisome that some layout tests that don't have fixed backgrounds have different repaint behavior.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:4920
> +            // scrolling tree propagates DescendantScrollersHaveSynchronousScrolling bits up the
> +            // tree, but shouldUpdateScrollLayerPositionSynchronously looks at the scrolling state
> +            // tree instead.)

Yeah that sucks.

Comment 8 Cameron McCormack (:heycam) 2021-06-01 22:43:36 PDT

(In reply to Simon Fraser (smfr) from comment #7)
> Seems OK, but it's worrisome that some layout tests that don't have fixed
> backgrounds have different repaint behavior.

I'll investigate that before landing.

Comment 9 Cameron McCormack (:heycam) 2021-06-02 18:52:13 PDT

>    bool hasSynchronousScrollingReasons(ScrollingNodeID nodeID) const { return !synchronousScrollingReasons(nodeID); }

It was a missing ! here. 😐

Comment 10 Cameron McCormack (:heycam) 2021-06-02 19:56:01 PDT

Created attachment 430438 [details]
Patch

Comment 11 EWS 2021-06-03 14:26:35 PDT

Committed r278419 (238443@main): <https://commits.webkit.org/238443@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430438 [details].