Summary: | [css-contain] Support contain:style for counters | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rob Buis <rbuis> | ||||||||||||||||||||
Component: | CSS | Assignee: | Rob Buis <rbuis> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | changseok, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, koivisto, kondapallykalyan, macpherson, menard, pdr, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | Other | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 172026 | ||||||||||||||||||||||
Attachments: |
|
Description
Rob Buis
2021-05-31 00:47:56 PDT
Created attachment 430185 [details]
Patch
Created attachment 430187 [details]
Patch
Created attachment 430680 [details]
Patch
Created attachment 430681 [details]
Patch
Created attachment 430717 [details]
Patch
Created attachment 430721 [details]
Patch
Created attachment 430750 [details]
Patch
Comment on attachment 430750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430750&action=review > Source/WebCore/rendering/RenderCounter.cpp:64 > + Element* ancestor = is<PseudoElement>(element) ? downcast<PseudoElement>(element).hostElement() : element.parentElement(); > + while (ancestor) { > + if (auto* style = ancestor->existingComputedStyle()) { I suppose this doesn't just traverse the render tree and get the style from there because you can have style containment in a non-rendered 'display:contents' element? Is that covered by tests? > Source/WebCore/rendering/RenderCounter.cpp:68 > + ancestor = ancestor->parentElement(); This should probably be doing composed tree traversal (parentInComposedTree). Of course the existing counter code doesn't do that either (so counters don't work accross shadow boundaries). At least this deserves a comment. > Source/WebCore/rendering/RenderCounter.cpp:87 > + Element* styleContainAncestor = ancestorStyleContainmentObject(*renderer.element()); > + > + while (true) { > + while (previous && !previous->renderer()) > + previous = ElementTraversal::previousIncludingPseudo(*previous); > + if (!previous) > + return nullptr; > + Element* previousStyleContainAncestor = ancestorStyleContainmentObject(*previous); > + if (previousStyleContainAncestor == styleContainAncestor) These traversal functions take optional 'stayWithin' parameter which I think should here be the styleContainAncestor. This avoids searching for the ancestor repeatedly. > Source/WebCore/rendering/RenderCounter.cpp:578 > + bool crossedBoundary = false; Which boundary? This could use a better name. > Source/WebCore/rendering/RenderCounter.cpp:580 > + crossedBoundary |= shouldApplyStyleContainment(*descendant); using logical operator instead of a bitwise one will avoid running the function repeatedly crossedBoundary = crossedBoundary || shouldApplyStyleContainment(*descendant); > Source/WebCore/rendering/RenderObject.cpp:2520 > + return renderer.style().containsStyle() && (!renderer.isInline() || renderer.isAtomicInlineLevelBox()) && !renderer.isRubyText() && (!renderer.isTablePart() || renderer.isTableCaption()) && !renderer.isTable(); if (!renderer.style().containsStyle()) return false; if (renderer.isInline() && !renderer.isAtomicInlineLevelBox()) return false; ...etc organised in some readable manner > Source/WebCore/rendering/style/RenderStyle.h:531 > bool containsLayout() const { return m_rareNonInheritedData->contain.contains(Containment::Layout); } > bool containsSize() const { return m_rareNonInheritedData->contain.contains(Containment::Size); } > + bool containsStyle() const { return m_rareNonInheritedData->contain.contains(Containment::Style); } Maybe these would read better as hasLayoutContainment()/hasSizeContainment()/hasStyleContainmen()? Comment on attachment 430750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430750&action=review > Source/WebCore/ChangeLog:4 > + [css-contain] Support contain:style > + https://bugs.webkit.org/show_bug.cgi?id=226458 Besides counters the spec talks about quotes and this patch doesn't address those. The title should be updated to reflect that this is not a complete implementation. The relevant quote code is in RenderTreeUpdater::GeneratedContent::updateQuotesUpTo Created attachment 442017 [details]
Patch
Comment on attachment 430750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430750&action=review >> Source/WebCore/ChangeLog:4 >> + https://bugs.webkit.org/show_bug.cgi?id=226458 > > Besides counters the spec talks about quotes and this patch doesn't address those. The title should be updated to reflect that this is not a complete implementation. > > The relevant quote code is in RenderTreeUpdater::GeneratedContent::updateQuotesUpTo I updated the title and opened https://bugs.webkit.org/show_bug.cgi?id=232083 to track the quote part. >> Source/WebCore/rendering/RenderCounter.cpp:64 >> + if (auto* style = ancestor->existingComputedStyle()) { > > I suppose this doesn't just traverse the render tree and get the style from there because you can have style containment in a non-rendered 'display:contents' element? Is that covered by tests? Yes. I believe this is covered by contain-style-counters-002.html. >> Source/WebCore/rendering/RenderCounter.cpp:68 >> + ancestor = ancestor->parentElement(); > > This should probably be doing composed tree traversal (parentInComposedTree). Of course the existing counter code doesn't do that either (so counters don't work accross shadow boundaries). At least this deserves a comment. Done. >> Source/WebCore/rendering/RenderCounter.cpp:87 >> + if (previousStyleContainAncestor == styleContainAncestor) > > These traversal functions take optional 'stayWithin' parameter which I think should here be the styleContainAncestor. This avoids searching for the ancestor repeatedly. Done. This may actually have fixed one more test. >> Source/WebCore/rendering/RenderCounter.cpp:578 >> + bool crossedBoundary = false; > > Which boundary? This could use a better name. Done. >> Source/WebCore/rendering/RenderCounter.cpp:580 >> + crossedBoundary |= shouldApplyStyleContainment(*descendant); > > using logical operator instead of a bitwise one will avoid running the function repeatedly > > crossedBoundary = crossedBoundary || shouldApplyStyleContainment(*descendant); Done. >> Source/WebCore/rendering/RenderObject.cpp:2520 >> + return renderer.style().containsStyle() && (!renderer.isInline() || renderer.isAtomicInlineLevelBox()) && !renderer.isRubyText() && (!renderer.isTablePart() || renderer.isTableCaption()) && !renderer.isTable(); > > if (!renderer.style().containsStyle()) > return false; > if (renderer.isInline() && !renderer.isAtomicInlineLevelBox()) > return false; > ...etc organised in some readable manner I was only partially able to do this. >> Source/WebCore/rendering/style/RenderStyle.h:531 >> + bool containsStyle() const { return m_rareNonInheritedData->contain.contains(Containment::Style); } > > Maybe these would read better as > > hasLayoutContainment()/hasSizeContainment()/hasStyleContainmen()? Good suggestion, but I think it is for another patch. Committed r284642 (243362@main): <https://commits.webkit.org/243362@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 442017 [details]. Comment on attachment 442017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442017&action=review > Source/WebCore/rendering/RenderCounter.cpp:88 > + Element* previousStyleContainAncestor = ancestorStyleContainmentObject(*previous); > + if (previousStyleContainAncestor == styleContainAncestor) The idea was to remove this stuff. You are staying within the styleContainAncestor by providing it to previousIncludingPseudo so the test here should never fail. In fact I think the whole outer loop can go away. Reopening to attach new patch. Created attachment 442303 [details]
Patch
Comment on attachment 442017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442017&action=review >> Source/WebCore/rendering/RenderCounter.cpp:88 >> + if (previousStyleContainAncestor == styleContainAncestor) > > The idea was to remove this stuff. You are staying within the styleContainAncestor by providing it to previousIncludingPseudo so the test here should never fail. > > In fact I think the whole outer loop can go away. Oops! Yes I did not see it earlier but indeed this is trying to do too much. Fixed. Committed r284755 (243464@main): <https://commits.webkit.org/243464@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 442303 [details]. |