| Summary: | scrollIntoView() erroneously scrolls non-containing block scrollers | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||
| Component: | Scrolling | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | darin, esprehn+autocc, ews-watchlist, fred.wang, glenn, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer, zalan | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | Safari Technology Preview | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Needs to share code with scroll latching ancestor finding. Created attachment 394941 [details]
Patch
Comment on attachment 394941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394941&action=review > Source/WebCore/rendering/RenderLayer.cpp:1922 > + for (RenderLayer* nextLayer = enclosingContainingBlockLayer(*this, crossFrameBoundaries);nextLayer; nextLayer = enclosingContainingBlockLayer(*nextLayer, crossFrameBoundaries)) { Missing space after semicolon. Also, why not use auto instead of type name here? > Source/WebCore/rendering/RenderLayer.cpp:2896 > + auto* enclosingLayer = ownerElement->renderer()->enclosingLayer(); > + if (enclosingLayer) Why not put this inside the if? Created attachment 394963 [details]
Patch
Created attachment 394964 [details]
Patch
Created attachment 394966 [details]
Patch
Comment on attachment 394966 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394966&action=review > Source/WebKitLegacy/win/WebView.cpp:2041 > + if (m_gestureTargetNode && m_gestureTargetNode->renderer() && m_gestureTargetNode->renderer()->enclosingLayer()) { > + if (auto* layer = m_gestureTargetNode->renderer()->enclosingLayer()) Seems like a double null check on enclosingLayer. (In reply to Darin Adler from comment #7) > Comment on attachment 394966 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394966&action=review > > > Source/WebKitLegacy/win/WebView.cpp:2041 > > + if (m_gestureTargetNode && m_gestureTargetNode->renderer() && m_gestureTargetNode->renderer()->enclosingLayer()) { > > + if (auto* layer = m_gestureTargetNode->renderer()->enclosingLayer()) > > Seems like a double null check on enclosingLayer. Oops yeah. Will fix. Created attachment 394971 [details]
Patch
Committed r259248: <https://trac.webkit.org/changeset/259248> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394971 [details]. |
Created attachment 394857 [details] Testcase RenderLayer::scrollRectToVisible() walks the parent chain and calls scrollIntoView() on each enclosing layer. But this is wrong, because not all parent layers are in the containing block chain. Attached testcase shows the bug.