Bug 60305 – Separate scrolling code out of RenderLayer

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

Most RenderLayers are not created for scrolling reasons. RenderLayer should not inherit from ScrollableArea; rather, it should own one when necessary. This would significantly reduce RenderLayer bloat.

[Simon Fraser (smfr)]

Created attachment 92512 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 92512 [details]
Patch

I haven't done extensive testing of this patch. Feel free to take it and do so.

[Simon Fraser (smfr)]

Taking a safer approach via 60327 for now, but we still think it's a good direction to separate scrolling code out of RenderLayer.

[Adam Barth]

Comment on attachment 92512 [details]
Patch

This patch causes many test failures:


Regressions: Unexpected text diff mismatch : (20)
  fast/block/float/marquee-shrink-to-avoid-floats.html = TEXT
  fast/css/MarqueeLayoutTest.html = TEXT
  fast/css/text-overflow-ellipsis-bidi.html = TEXT
  fast/css/text-overflow-ellipsis-strict.html = TEXT
  fast/css/text-overflow-ellipsis.html = TEXT
  fast/events/autoscroll-in-textfield.html = TEXT
  fast/forms/date-input-visible-strings.html = TEXT
  fast/forms/input-readonly-autoscroll.html = TEXT
  fast/forms/search-rtl.html = TEXT
  fast/html/marquee-scroll.html = TEXT
  fast/html/marquee-scrollamount.html = TEXT
  fast/inline-block/003.html = TEXT
  fast/repaint/intermediate-layout-position-clip.html = TEXT
  fast/repaint/line-in-scrolled-clipped-block.html = TEXT
  fast/repaint/text-selection-rect-in-overflow-2.html = TEXT
  fast/repaint/text-selection-rect-in-overflow.html = TEXT
  fast/table/colspanMinWidth-vertical.html = TEXT
  fast/transforms/scrollIntoView-transformed.html = TEXT
  inspector/console/console-command-clear.html = TEXT
  svg/custom/scroll-hit-test.xhtml = TEXT

[Simon Fraser (smfr)]

Yeah, it wasn't meant for review.

[Simon Fraser (smfr)]

Not an issue after fixing 42550.

[Julien Chaffraix]

Created attachment 119439 [details]
WIP patch 1: rebaselined, cleaned-up, still some rough edges butcomments welcome.

[Julien Chaffraix]

Created attachment 119780 [details]
Proposed refactoring 1: Introduce RenderLayerScrollableArea.

[Julien Chaffraix]

Created attachment 136315 [details]
Proposed refactoring 2: Updated on ToT, tweaked the logic a bit.

[Julien Chaffraix]

For the record, this was discussed briefly as part of the scrolling session at this year's contributors meeting (http://trac.webkit.org/wiki/Scrolling%20Session%20Meeting%202012).

Sam / Anders, I hope the talk answered your concerns over this patch and you can find some time to look at it. The idea is to move everything scrolling and resizing related to the new RenderLayerScrollableArea (maybe we can come up with a better naming for that though like ScrollingHandler)

[Julien Chaffraix]

Comment on attachment 136315 [details]
Proposed refactoring 2: Updated on ToT, tweaked the logic a bit.

The patch won't apply anymore and I have switched to other tasks so clearing the review flag.

[Antonio Gomes]

(In reply to comment #3)
> Taking a safer approach via 60327 for now, but we still think it's a good direction to separate scrolling code out of RenderLayer.

Simon/Julien. I would to take on this patch. Should it be fine?

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

I'll upload newer revisions from the patch from bug 215443 here, per request on Simon. Good idea to re-use this much older ticket for this :-)

First need to get build running for all platforms, please bear with me.

[Nikolas Zimmermann]

Created attachment 406553 [details]
Patch

[Nikolas Zimmermann]

Created attachment 406558 [details]
Patch

[Nikolas Zimmermann]

Created attachment 406578 [details]
Patch

[Nikolas Zimmermann]

Created attachment 406581 [details]
Patch

[Darin Adler]

Comment on attachment 406581 [details]
Patch

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

Seems like probably a great idea as long as it doesn’t add too much virtual function overhead. Love to hear what Simon thinks.

Is there some way to make this change in smaller steps, for example adding a scrollableLayer function that’s a synonym for layer first and deploying it while they still both return the same thing, so it’s easier to see the final step that splits the classes is correct? Even adding a scrollableLayer function to RenderLayer that returns "this" as a similar staging step. Could even add a RenderLayerScrollable.h header that just includes RenderLayer.h as a similar staging step.

> Source/WebCore/page/ios/FrameIOS.mm:824
> -    RenderLayer& layer = *downcast<RenderBoxModelObject>(*renderer).layer();
> +    auto* layer = downcast<RenderBoxModelObject>(*renderer).scrollableLayer();
> +    if (!layer)
> +        return;

I noticed that in some other cases we were able to assert rather than checking null. Not sure why here we had to check for null.

Could we add a hasScrollableLayer function to the renderer rather than adding this null check?

[Nikolas Zimmermann]

Created attachment 406614 [details]
Patch

[Nikolas Zimmermann]

Created attachment 406623 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 406623 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:2415
> +                auto* scrollableLayer = layer->scrollableLayer();

I think having scrollableLayer() on RenderLayer is confusing. I'd prefer to see this written as:

if (layer->isScrollable())
  auto& scrollableLayer = downcast<RenderLayerScrollable>(&layer).

[Nikolas Zimmermann]

Created attachment 406636 [details]
Patch

[Nikolas Zimmermann]

(In reply to Simon Fraser (smfr) from comment #22)
> 
> I think having scrollableLayer() on RenderLayer is confusing. I'd prefer to
> see this written as:
> 
> if (layer->isScrollable())
>   auto& scrollableLayer = downcast<RenderLayerScrollable>(&layer).

Hehe, I had that in my last variant :-) Will first try to get this to build everywhere, only iOS missing. Then will work on improving the readability - I agree, it really reads odd 'layer.scrollableLayer()'.

[Nikolas Zimmermann]

Created attachment 406678 [details]
Patch

[Nikolas Zimmermann]

Created attachment 406688 [details]
Patch

[Nikolas Zimmermann]

Created attachment 406712 [details]
Patch, v2

Remove scrollableLayer() accessors, use downcast directly instead, as suggested by Simon

[Nikolas Zimmermann]

Created attachment 406713 [details]
Patch, v3

Fix typo in DOMHTML.mm - should fix iOS/tv/watch builds again

[Nikolas Zimmermann]

Created attachment 406724 [details]
Patch, v4

Fix build in WebFrame.mm - hopefully the last iOS/tv/watch specific issue

[Said Abou-Hallawa]

Comment on attachment 406724 [details]
Patch, v4

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

After looking at some of the changes in this patch, I felt annoyed by seeing the same pattern:

Original:

    layer->doSomething();

New:

    if (is<RenderLayerScrollable>(layer)) {
        auto* scrollableLayer = downcast<RenderLayerScrollable>(layer);
        scrollableLayer->doSomething();
    }

RenderLayerScrollable is a superclass of RenderLayer, so why does not RenderLayer define doSomething() a virtual method and RenderLayerScrollable override it? I think this would leave most of the code unchanged. I understand the virtual method adds indirection calling overhead, but are not the if-statement and the casting add similar or even more expensive overhead as well?

class RenderLayer {
public:
    virtual void doSomething() { };
};

class ScrollArea {
public:
    void doSomething() { ... }
};

class RenderLayerSrollable : public RenderLayer, public ScrollArea {
public:
    void doSomething() override { ScrollArea::doSomething(); }
};

Maybe another approach is to make RenderLayer contain a pointer to ScrollableArea or a superclass of it, which Simon did in his original patch. In this case, no virtual table will be needed. But some of the calls to RenderLayer will be directed to ScrollableArea.

class ScrollArea {
public:
    void doSomething() { ... }
};

class RenderLayer {
public:
    void doSomething()
    {
        if (m_scrollableArea)
            m_scrollArea->doSomething();
    };
    
private:
    std::unique_ptr<RenderLayerScrollableArea> m_scrollableArea;
};

> Source/WebCore/rendering/RenderLayerScrollable.h:60
> +    friend class RenderLayer;

Why RenderLayer is the base of this class and a friend at the same time?

[Nikolas Zimmermann]

(In reply to Said Abou-Hallawa from comment #30)

Thanks Said for the comments.

> Original:
> 
>     layer->doSomething();
> 
> New:
> 
>     if (is<RenderLayerScrollable>(layer)) {
>         auto* scrollableLayer = downcast<RenderLayerScrollable>(layer);
>         scrollableLayer->doSomething();
>     }
> 
> RenderLayerScrollable is a superclass of RenderLayer, so why does not
> RenderLayer define doSomething() a virtual method and RenderLayerScrollable
> override it? I think this would leave most of the code unchanged. I
> understand the virtual method adds indirection calling overhead, but are not
> the if-statement and the casting add similar or even more expensive overhead
> as well?

When I first split RenderLayer in two parts (RenderLayerHTML/SVG in the SVG PoC branch), I did just that: used virtual methods, avoiding all if(is<>) / downcast constructs.

While profiling the SVG PoC branch I found that by removing all virtual calls and employing the LayerType concept I could gain a speed-up - I can look up the profiles, I think it was a ~ 7% progression on MotionMark SVG suites compared to the SVG PoC branch before with virtual function calls.

The is<RenderLayerScrollable>() boils down to one inlined function call to access RenderLayer::layerType() and one comparison (== LayerType::Scrollable).
The downcast<> boils down to a static_cast, which is cheap (zero for single-inheritance relations, less higher for N base classes).

Anyhow, I did not perform the performance analysis on this branch so cannot proof that this is faster - I'd need to repeat the exercise, but I doubt it will be faster :-)

> 
> Maybe another approach is to make RenderLayer contain a pointer to
> ScrollableArea or a superclass of it, which Simon did in his original patch.
> In this case, no virtual table will be needed.
> But some of the calls to RenderLayer will be directed to ScrollableArea.

I wasn't aware of his patch when I started to work on this.
It boils down to: do we want a is-a or has-a relationship? Currently we assume that the RenderLayer is-a scrollable area, and I've adapted that.

I haven't explored has-a yet, maybe I'm overlooking other benefits.
 
> > Source/WebCore/rendering/RenderLayerScrollable.h:60
> > +    friend class RenderLayer;
> 
> Why RenderLayer is the base of this class and a friend at the same time?
Probably a leftover, will check.

[Nikolas Zimmermann]

Created attachment 414792 [details]
Patch, v5 (squashed for testing)

NOTE: Set r? to trigger EWS - don't know if that's necessary, not intended for a real review yet.

"Patch, v5" actually consists of four independent patches, that can be reviewed/landed separated.
I squashed them together to obtain EWS results first for the final patch, since I still expect regressions on Mac/Win etc. that need to be checked first. Once EWS shows no regressions, I'll start uploading the individual patch chunks for review. Feel free to check the patch nevertheless. It is based on the previous iterations + discussions after the meeting with Simon, Said, etc.

The four independent patches are:
1) Introduce RenderLayerScrollableArea stub (build system changes, etc.)
2) Move overflow/scrolling functionality from RenderLayer -> RenderLayerScrollableArea.
   Introduce temporary glue code: void RenderLayer::foo() { if (m_scrollableArea) m_scrollableArea->foo(); }
   to minimize the changes to WebCore in general and ease review.
3) Remove glue code, make WebCore aware of RenderLayerScrollableArea
4) Enable dynamic creation of RenderLayerScrollableArea, only when it is needed!
   This is the tricky part that kept me busy for a long while.

If a scrollable area is not needed for a RenderLayer, the RenderLayer will now consume less memory than before.
I measured this to be performance and memory-neutral on two benchmarks: MotionMark, Speedometer.

While the size reduction of RenderLayer is significant, it is negligible as soon as the layer needs a backing store...
Thus I expect this to have little impact on memory consumption in "real-life" use-cases. Nevertheless it simplifies the code a lot, and opens possibilities for further refactoring & cleanup that are the main motivation for this work: e.g. introduce RenderLayerHTML, further cleaning up RenderLayer.

The final goal is to make RenderLayer less expensive to use, to allow to be reused for SVG.
I have a PoC branch where SVG rendering/hit-testing/etc. goes through RenderLayer, which opens up nice possibilites, such as 3D transformations, GPU composition etc. However on static rendering it currently regresses performance, due to the RenderLayer cost.

Disclaimer: I just finished rebasing this on ToT -- it was ~ 2 months behind. Only ran a limited set of layout tests, as I want to get this patch out quickly to get some early results before EWS is shut-down due to the maintenance tasks this week.
.

[Nikolas Zimmermann]

Created attachment 414793 [details]
Patch, v5 (squashed for testing)

[Nikolas Zimmermann]

Created attachment 414887 [details]
Patch, v6 (squashed for testing)

[Nikolas Zimmermann]

Created attachment 414891 [details]
Patch, v8 (squashed for testing)

[Nikolas Zimmermann]

Created attachment 414893 [details]
Patch, v9 (squashed for testing)

[Nikolas Zimmermann]

Created attachment 414894 [details]
Patch, v10 (squashed for testing)

[Nikolas Zimmermann]

Created attachment 414900 [details]
Patch, v11 (squashed for testing)

[Nikolas Zimmermann]

Created attachment 414901 [details]
Patch, v12 (squashed for testing)

[Nikolas Zimmermann]

Created attachment 414907 [details]
Patch, v13 (squashed for testing)

Hopefully "Patch, v13" compiles everywhere. Willn now aggregate and fix remaining layout test regressions/crashes on all platforms. Stay tuned.

[Nikolas Zimmermann]

Created attachment 414917 [details]
Patch, v14 (squashed for testing)

[Nikolas Zimmermann]

Created attachment 414922 [details]
Patch, v15 (squashed for testing)

"Patch, v15" should compile everywhere and show no crashes. Three layout test regressions are expected to remain on all platforms, that are tackled once the crashes are gone.

[Nikolas Zimmermann]

Created attachment 414974 [details]
Patch, v16 (squashed for testing)

[Nikolas Zimmermann]

It took a while to setup a local macOS/iOS debugging environment, and debug the compositing test failures -- I now have a clear understanding what went wrong and only one remaining regressions to tackle -- a final patch can be expected today or tomorrow.

[Nikolas Zimmermann]

Created attachment 416053 [details]
Patch, v17 (squashed for testing)

[Nikolas Zimmermann]

Created attachment 416058 [details]
Patch, v18 (squashed for testing)

[Nikolas Zimmermann]

I've fixes the remaining layout tests failures on macOS/iOS using a catalina system. Assuming patch v18 passes EWS everywhere, I consider the initial work done.

As the patch is really large, I propose to split up the patch into 4 atomic chunks:

1) Add RenderLayerScrollableArea stub + build system changes (already filed https://bugs.webkit.org/show_bug.cgi?id=219808 for this)

2) Move overflow/scroll/marquee/... code from RenderLayer to RenderLayerScrollableArea in such a way that the impact outside of RenderLayer is minimal (to allow for a smaller patch, at the cost of a few proxy methods in RenderLayer that forward to RenderLayerScrollableArea)

3) Remove the proxy methods from RenderLayer, and make a few call-sites (EventHandler, ...) aware of it

4) Only create RenderLayerScrollableArea on demand.

The patches for 1) + 2) + 3) shall have no observable difference.
Only patch 4) actually enables the benefit of the improved design.

See RenderBox::requiresLayerWithScrollableArea() for the small list of conditions: RenderView + document element renderer receive a RenderLayerScrollableArea (RLSA). Boxes with horizontal/vertical overflow present, CSS 'resize' property set, or <marquee> elements also get a RLSA -- all other cases save memory.

Let me know what you think of this.

[Nikolas Zimmermann]

Created attachment 416978 [details]
Patch, v19

[Nikolas Zimmermann]

I've uploaded the full patch for review - unfortunately the part that moves methods from RenderLayer -> RenderLayerScrollableArea is ~ 130k alone + ChangeLog.

Before taking additional time to split up things, I'd like to hear from the reviewers first what they propose -- keep this large patch, or add temporary glue methods to minimize the changes outside of RenderLayer (but this glue code would be removed again in follow-up patches...)

[Nikolas Zimmermann]

Created attachment 416980 [details]
Patch, v20

[Darin Adler]

Comment on attachment 416980 [details]
Patch, v20

I like the "temporary glue" idea if it can effectively keep the large code movement separate from the other changes. Great for bisecting later.

[Nikolas Zimmermann]

(In reply to Darin Adler from comment #51)
> Comment on attachment 416980 [details]
> Patch
> 
> I like the "temporary glue" idea if it can effectively keep the large code
> movement separate from the other changes. Great for bisecting later.

Good, I will adapt the patch!
Thanks Darin, and sorry for the late reply - I missed the bugzilla mail.

[Nikolas Zimmermann]

Created attachment 417571 [details]
Patch, v21

[Nikolas Zimmermann]

Created attachment 417573 [details]
Patch, v22

[Nikolas Zimmermann]

"Patch, v22" aims to provide the same functionality than the previous patches, but the code changes are more isolated in RenderLayer/RenderLayerScrollableArea.

Important public RenderLayerScrollableArea methods are proxied through RenderLayer (temporary glue code) to minimize code changes outside of RenderLayer. The only code changes that have to be made is in places that assume that a RenderLayer* can be casted to a ScrollableArea*, which is no longer the case.

This is the smallest version I could produce, obviously still a large patch...

Just finished rebasing this on WebKit ToT, and haven't performed a full layout test run yet after rebasing. Will get some sleep, but didn't want to leave without giving EWS a chance to work overnight :-)

[Nikolas Zimmermann]

Created attachment 417666 [details]
Patch, v23

[Simon Fraser (smfr)]

Comment on attachment 417666 [details]
Patch, v23

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

> Source/WebCore/rendering/RenderLayerScrollableArea.cpp:90
> +    auto& renderer = m_layer.renderer();
> +    ASSERT(m_registeredScrollableArea == renderer.view().frameView().containsScrollableArea(this));
> +
> +    if (m_registeredScrollableArea)
> +        renderer.view().frameView().removeScrollableArea(this);
> +
> +#if ENABLE(IOS_TOUCH_EVENTS)
> +    unregisterAsTouchEventListenerForScrolling();
> +#endif
> +    if (Element* element = renderer.element())
> +        element->setSavedLayerScrollPosition(m_scrollPosition);
> +
> +    destroyScrollbar(HorizontalScrollbar);
> +    destroyScrollbar(VerticalScrollbar);
> +
> +    if (auto* scrollingCoordinator = renderer.page().scrollingCoordinator())
> +        scrollingCoordinator->willDestroyScrollableArea(*this);
> +
> +    clearScrollCorner();
> +    clearResizer();

Doing all this in the destructor makes me a bit nervous. Can we do it in a "clear" function that's called from RenderLayer?

> Source/WebCore/rendering/RenderLayerScrollableArea.cpp:106
> +void RenderLayerScrollableArea::storeScrollPosition()
> +{
> +    auto* element = m_layer.renderer().element();
> +    if (!element)
> +        return;
> +
> +    if (m_layer.renderBox()) {
> +        // We save and restore only the scrollOffset as the other scroll values are recalculated.
> +        m_scrollPosition = element->savedLayerScrollPosition();
> +        if (!m_scrollPosition.isZero())
> +            scrollAnimator().setCurrentPosition(m_scrollPosition);
> +    }
> +
> +    element->setSavedLayerScrollPosition(IntPoint());

"Store" is ambiguous here. It's more like "restore".

> Source/WebCore/rendering/RenderLayerScrollableArea.h:277
> +    // Minimum padding on x86_64 (4 bytes), after m_scrollHeight)
> +    bool m_scrollDimensionsDirty : 1;
> +    bool m_inOverflowRelayout : 1;
> +    bool m_registeredScrollableArea : 1;
> +    bool m_hasCompositedScrollableOverflow : 1;
> +
> +#if PLATFORM(IOS_FAMILY)
> +#if ENABLE(IOS_TOUCH_EVENTS)
> +    bool m_registeredAsTouchEventListenerForScrolling : 1;
> +#endif
> +    bool m_adjustForIOSCaretWhenScrolling : 1;
> +#endif
> +    bool m_requiresScrollPositionReconciliation : 1;
> +    bool m_containsDirtyOverlayScrollbars : 1;
> +    bool m_updatingMarqueePosition : 1;

It's probably not worth using a bitset here for memory saving any more; if these are plain bools you can use initializers.

[Nikolas Zimmermann]

(In reply to Simon Fraser (smfr) from comment #57)
> Comment on attachment 417666 [details]
> Patch, v23
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417666&action=review
> 
> > Source/WebCore/rendering/RenderLayerScrollableArea.cpp:90
> > +    auto& renderer = m_layer.renderer();
> > +    ASSERT(m_registeredScrollableArea == renderer.view().frameView().containsScrollableArea(this));
> > +
> > +    if (m_registeredScrollableArea)
> > +        renderer.view().frameView().removeScrollableArea(this);
> > +
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> > +    unregisterAsTouchEventListenerForScrolling();
> > +#endif
> > +    if (Element* element = renderer.element())
> > +        element->setSavedLayerScrollPosition(m_scrollPosition);
> > +
> > +    destroyScrollbar(HorizontalScrollbar);
> > +    destroyScrollbar(VerticalScrollbar);
> > +
> > +    if (auto* scrollingCoordinator = renderer.page().scrollingCoordinator())
> > +        scrollingCoordinator->willDestroyScrollableArea(*this);
> > +
> > +    clearScrollCorner();
> > +    clearResizer();
> 
> Doing all this in the destructor makes me a bit nervous. Can we do it in a
> "clear" function that's called from RenderLayer?
Good idea. RenderLayer::clearLayerScrollableArea() already calls clearScrollCorner / clearResizer on RenderLayerScrollableArea. I'll change that to a single clear() call that encapsulates what the destructor is currenly doing.

> > Source/WebCore/rendering/RenderLayerScrollableArea.cpp:106
> > +void RenderLayerScrollableArea::storeScrollPosition()
> > +{
> > +    auto* element = m_layer.renderer().element();
> > +    if (!element)
> > +        return;
> > +
> > +    if (m_layer.renderBox()) {
> > +        // We save and restore only the scrollOffset as the other scroll values are recalculated.
> > +        m_scrollPosition = element->savedLayerScrollPosition();
> > +        if (!m_scrollPosition.isZero())
> > +            scrollAnimator().setCurrentPosition(m_scrollPosition);
> > +    }
> > +
> > +    element->setSavedLayerScrollPosition(IntPoint());
> 
> "Store" is ambiguous here. It's more like "restore".
Indeed, bad naming, fixed.
> 
> > Source/WebCore/rendering/RenderLayerScrollableArea.h:277
> > +    // Minimum padding on x86_64 (4 bytes), after m_scrollHeight)
> > +    bool m_scrollDimensionsDirty : 1;
> > +    bool m_inOverflowRelayout : 1;
> > +    bool m_registeredScrollableArea : 1;
> > +    bool m_hasCompositedScrollableOverflow : 1;
> > +
> > +#if PLATFORM(IOS_FAMILY)
> > +#if ENABLE(IOS_TOUCH_EVENTS)
> > +    bool m_registeredAsTouchEventListenerForScrolling : 1;
> > +#endif
> > +    bool m_adjustForIOSCaretWhenScrolling : 1;
> > +#endif
> > +    bool m_requiresScrollPositionReconciliation : 1;
> > +    bool m_containsDirtyOverlayScrollbars : 1;
> > +    bool m_updatingMarqueePosition : 1;
> 
> It's probably not worth using a bitset here for memory saving any more; if
> these are plain bools you can use initializers.
Good point - by this I spotted that m_scrollWidth / m_scrollHeight were not initialized in the constructor -- moved everything to in-place initialization to avoid that.

[Nikolas Zimmermann]

Comment on attachment 417666 [details]
Patch, v23

Landed in r271559 (https://trac.webkit.org/changeset/271559/webkit).
Leaving the bug open, as the glue code removal is now pending.

[Truitt Savell]

The changes in https://trac.webkit.org/changeset/271559/webkit

broke fast/forms/password-scrolled-after-caps-lock-toggled.html on Big Sur

History;
https://results.webkit.org/?suite=layout-tests&test=fast%2Fforms%2Fpassword-scrolled-after-caps-lock-toggled.html

Diff:
--- /Volumes/Data/slave/bigsur-release-tests-wk2/build/layout-test-results/fast/forms/password-scrolled-after-caps-lock-toggled-expected.txt
+++ /Volumes/Data/slave/bigsur-release-tests-wk2/build/layout-test-results/fast/forms/password-scrolled-after-caps-lock-toggled-actual.txt
@@ -25,7 +25,7 @@
 PASS document.getElementById('input').scrollLeft is 0
 
 After caps lock enabled:
-PASS document.getElementById('input').scrollLeft is 0
+FAIL document.getElementById('input').scrollLeft should be 0. Was 5.
 
 After caps lock disabled:
 PASS document.getElementById('input').scrollLeft is 0
@@ -39,6 +39,7 @@
 After caps lock disabled:
 PASS document.getElementById('input').scrollLeft is 0
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE
 

I can reproduce this with command:
run-webkit-tests --iterations 2000 --exit-after-n-failures 1 --exit-after-n-crashes-or-timeouts 1 --debug-rwt-logging --no-retry --force --no-build -f fast/forms/password-scrolled-after-caps-lock-toggled.html

[Nikolas Zimmermann]

(In reply to Truitt Savell from comment #60)
> The changes in https://trac.webkit.org/changeset/271559/webkit
> 
> broke fast/forms/password-scrolled-after-caps-lock-toggled.html on Big Sur
> 
> History;
> https://results.webkit.org/?suite=layout-tests&test=fast%2Fforms%2Fpassword-
> scrolled-after-caps-lock-toggled.html
> 
Oh I missed this comment, sorry. webkit.org/b/220743 was opened to track the issue. The regression was fixed in r271732.

Please let me know if you see any other issues/flakiness since this landed.

[Nikolas Zimmermann]

Since this change was rather intrusive and large, I'd leave a comment here explaining the dependencies of the whole patchset, if we ever need to bisect this :-)

The chronological order of the RenderLayerScrollableArea introduction:

#1) webkit.org/b/219808 - Introduce RenderLayerScrollableArea
Landed: 30.12.2020
Committed r271111: <https://trac.webkit.org/changeset/271111>

#2) webkit.org/b/60305 - Separate scrolling code out of RenderLayer
Landed: 16.01.2021
Committed r271559: <https://trac.webkit.org/changeset/271559>

#3) webkit.org/b/220705 - Remove recently added glue code: RenderLayer::(scrollToOffset / scrollToXOffset / scrollToYOffset)
Landed: 18.01.2021
Committed r271577: <https://trac.webkit.org/changeset/271577>

#4) webkit.org/b/220708 - Remove recently added glue code: RenderLayer::(setAdjustForIOSCaretWhenScrolling / setScrollShouldClearLatchedState / setConstrainsScrollingToContentEdge)
Landed: 18.01.2021
Committed r271585: <https://trac.webkit.org/changeset/271585>

#5) webkit.org/b/220715 - Continue removing glue code from RenderLayer that was recently added in r271559
Landed: 19.01.2021
Committed r271598: <https://trac.webkit.org/changeset/r271598>

#6) webkit.org/b/220743 - REGRESSION (r271559): [BigSur] fast/forms/password-scrolled-after-caps-lock-toggled.html is consistently failing
Landed: 22.01.2021
Committed r271732: <https://trac.webkit.org/changeset/271732>

#7) webkit.org/b/220729 - Continue removing glue code from RenderLayer that was recently added in r271559
Landed: 22.01.2021
Committed r271740: <https://trac.webkit.org/changeset/r271740>

#8) webkit.org/b/220851 - Finish introduction of RenderLayerScrollableArea: remove remaining glue code from RenderLayer
Landed: 25.01.2021
Committed r271814: <https://trac.webkit.org/changeset/r271814>

Today the final patch landed - bug 220851 - removing the last bits of the temporary glue code and choosing a sane, consistent variable naming scheme for the scrollableArea() / ensureLayerScrollableArea() results (prefer 'scrollableArea' over 'scrollableLayer').

Therefore this bug can be closed now, finally. Now moving on to SVG + RenderLayer again.

[Radar WebKit Bug Importer]

<rdar://problem/73589122>