Summary: | offsetLeft and offsetParent should adjust across shadow boundaries | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
Component: | DOM | Assignee: | Ryosuke Niwa <rniwa> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | berto, cdumez, cgarcia, cmarcelo, dbates, dino, eoconnor, esprehn+autocc, ews-watchlist, gustavo, gyuyoung.kim, kangil.han, koivisto, kondapallykalyan, mcatanzaro, simon.fraser, thorton, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 148695 | ||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2016-05-06 17:12:25 PDT
Filed https://github.com/w3c/webcomponents/issues/497 for the spec work. Created attachment 357294 [details]
WIP
Almost there but I still have a bug that my loop can skip over the offset parent. Basically, we need to find the first offset parent which is a shadow inclusive ancestor so I might just need to check that directly instead of relying on the retargeting algorithm. FYI, our offsetParent algorithm doesn't match the spec https://github.com/w3c/csswg-drafts/issues/409, bug 161788. (In reply to Simon Fraser (smfr) from comment #5) > FYI, our offsetParent algorithm doesn't match the spec > https://github.com/w3c/csswg-drafts/issues/409, bug 161788. Oh, interesting. This patch mostly preserves the behavior of offsetParent though. Any quirks it has will be preserved. I'm basically walking offset parents until I find the shadow-including ancestor node. Created attachment 357478 [details]
Fixes the implementation
Created attachment 357503 [details]
Fixed GTK+ build failures
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API (In reply to Build Bot from comment #9) > Thanks for the patch. If this patch contains new public API please make sure > it follows the guidelines for new WebKit2 GTK+ API. See > http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Note that this patch does not add or remove a new GTK+ API. It simply updates its implementation to match the JavaScript behavior. Comment on attachment 357503 [details] Fixed GTK+ build failures View in context: https://bugs.webkit.org/attachment.cgi?id=357503&action=review > Source/WebCore/ChangeLog:9 > + Updates our implementation of offsetLeft, offsetTop, and offsetParent to match the latest discussion in CSS WG. Make it clear this is about offset* in shadow roots; you're not fixing the offsetParent algorithm in general. > Source/WebCore/dom/Element.cpp:914 > + auto parent = makeRefPtr(offsetParent()); > + if (!parent || !parent->isInShadowTree()) > + return offsetLeft(); > + > + ASSERT(&parent->document() == &document()); > + if (&parent->treeScope() == &treeScope()) > + return offsetLeft(); > + > + double offset = offsetLeft(); It would be nice to avoid the 3 separate call sites for offsetLeft(). You could move 'double offset = offsetLeft();' to after the updateLayoutIgnorePendingStylesheets(). > Source/WebCore/dom/Element.cpp:926 > + document().updateLayoutIgnorePendingStylesheets(); Sucks that both offsetLeft and offsetLeftForBindings() call updateLayoutIgnorePendingStylesheets(). (In reply to Simon Fraser (smfr) from comment #11) > Comment on attachment 357503 [details] > Fixed GTK+ build failures > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357503&action=review > > > Source/WebCore/ChangeLog:9 > > + Updates our implementation of offsetLeft, offsetTop, and offsetParent to match the latest discussion in CSS WG. > > Make it clear this is about offset* in shadow roots; you're not fixing the > offsetParent algorithm in general. Phrased as follows: Update the WebKit's treatment of shadow boundaries in offsetLeft, offsetTop, and offsetParent to match the latest discussion in CSS WG. > > Source/WebCore/dom/Element.cpp:914 > > + auto parent = makeRefPtr(offsetParent()); > > + if (!parent || !parent->isInShadowTree()) > > + return offsetLeft(); > > + > > + ASSERT(&parent->document() == &document()); > > + if (&parent->treeScope() == &treeScope()) > > + return offsetLeft(); > > + > > + double offset = offsetLeft(); > > It would be nice to avoid the 3 separate call sites for offsetLeft(). You > could move 'double offset = offsetLeft();' to after the > updateLayoutIgnorePendingStylesheets(). Done that. > > Source/WebCore/dom/Element.cpp:926 > > + document().updateLayoutIgnorePendingStylesheets(); > > Sucks that both offsetLeft and offsetLeftForBindings() call > updateLayoutIgnorePendingStylesheets(). We don't need this now that we always call offsetTop/offsetLeft upfront. Thanks for the review! Committed r239313: <https://trac.webkit.org/changeset/239313> |