Fix counter() and counters() in alt content [chromium/src : main]
0 views
Skip to first unread message
Rune Lillesveen (Gerrit)
Dec 4, 2025, 4:23:08 AM (6 days ago) Dec 4
to Daniil Sakhapov, Menard, Alexis, (Julie)Jeongeun Kim, Kevin Babbitt, Rune Lillesveen, Chromium LUCI CQ, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, zol...@webkit.org, abigailbk...@google.com, blink-rev...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, josiah...@chromium.org, kyungjunle...@google.com, lucasrada...@google.com, nektar...@chromium.org, yuzo+...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Daniil Sakhapov
Rune Lillesveen added 5 comments![]( "Open in Gerrit")
File third_party/blink/renderer/core/css/style_engine.cc
Line 884, Patchset 3 (Latest):
ContentData* content = pseudo_element->EnsureMutableAltContentData();Rune Lillesveen . unresolved
"Ensure" should never return nullptr. Perhaps something like CreateMutableAltContentDataForCountersIfNeeded()?
File third_party/blink/renderer/core/dom/pseudo_element.h
Line 132, Patchset 3 (Latest):
Member<ContentData> content_data_;Rune Lillesveen . unresolved
Does this happen often enough to justify an extra member on PseudoElement? Would adding it on rare-data be an option?
File third_party/blink/renderer/core/dom/pseudo_element.cc
Line 83, Patchset 3 (Latest):
bool HasAltCounterContent(const ContentData* content) {Rune Lillesveen . unresolved
Why not make this a function on ContentData?
Line 413, Patchset 3 (Latest):
builder.SetContent(const_cast<ContentData*>(style.GetContentData()));Rune Lillesveen . unresolved
Does SetContent() need a mutable ContentData?
File third_party/blink/renderer/core/layout/layout_object_hot.cc
Line 174, Patchset 3 (Latest):
? const_cast<StyleImage*>(Rune Lillesveen . unresolved
So ContentData actually needs to be mutable? Or is this a case of "let's stop here because it requires changes everywhere"?
Related details
Attention is currently required from:
- Daniil Sakhapov
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Dec 4, 2025, 7:24:55 AM (6 days ago) Dec 4
to Menard, Alexis, (Julie)Jeongeun Kim, Kevin Babbitt, Rune Lillesveen, Chromium LUCI CQ, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, zol...@webkit.org, abigailbk...@google.com, blink-rev...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, josiah...@chromium.org, kyungjunle...@google.com, lucasrada...@google.com, nektar...@chromium.org, yuzo+...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Rune Lillesveen
Daniil Sakhapov voted and added 5 comments![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
5 comments
File third_party/blink/renderer/core/css/style_engine.cc
Line 884, Patchset 3:
ContentData* content = pseudo_element->EnsureMutableAltContentData();Rune Lillesveen . resolved
"Ensure" should never return nullptr. Perhaps something like CreateMutableAltContentDataForCountersIfNeeded()?
Daniil Sakhapov
Done
File third_party/blink/renderer/core/dom/pseudo_element.h
Does this happen often enough to justify an extra member on PseudoElement? Would adding it on rare-data be an option?
Daniil Sakhapov
Done
File third_party/blink/renderer/core/dom/pseudo_element.cc
Line 83, Patchset 3:
bool HasAltCounterContent(const ContentData* content) {Rune Lillesveen . resolved
Why not make this a function on ContentData?
Daniil Sakhapov
Done
Line 413, Patchset 3:
builder.SetContent(const_cast<ContentData*>(style.GetContentData()));Rune Lillesveen . unresolved
Does SetContent() need a mutable ContentData?
Daniil Sakhapov
See the other comment + I couldn't easily change css_properties.json5 to make content immutable.
Can be a separate CL?
File third_party/blink/renderer/core/layout/layout_object_hot.cc
Line 174, Patchset 3:
? const_cast<StyleImage*>(Rune Lillesveen . unresolved
So ContentData actually needs to be mutable? Or is this a case of "let's stop here because it requires changes everywhere"?
Daniil Sakhapov
The let's stop here case for my CL, but it seems like some image resource observer also abuses content mutability.
But to fix that sounds like a separate CL?
Related details
Attention is currently required from:
- Rune Lillesveen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Dec 4, 2025, 7:40:19 AM (6 days ago) Dec 4
to Daniil Sakhapov, Menard, Alexis, (Julie)Jeongeun Kim, Kevin Babbitt, Rune Lillesveen, Chromium LUCI CQ, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, zol...@webkit.org, abigailbk...@google.com, blink-rev...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, josiah...@chromium.org, kyungjunle...@google.com, lucasrada...@google.com, nektar...@chromium.org, yuzo+...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Daniil Sakhapov
Rune Lillesveen added 2 comments![]( "Open in Gerrit")
File third_party/blink/renderer/core/dom/pseudo_element.cc
Line 413, Patchset 3:
builder.SetContent(const_cast<ContentData*>(style.GetContentData()));Rune Lillesveen . unresolved
Daniil SakhapovDoes SetContent() need a mutable ContentData?
See the other comment + I couldn't easily change css_properties.json5 to make content immutable.
Can be a separate CL?
Rune Lillesveen
Yes. Then I'd prefer to remove the constness of the ContentData returned from ComputedStyle. I think that's better than having const_casts.
File third_party/blink/renderer/core/layout/layout_object_hot.cc
Line 174, Patchset 3:
? const_cast<StyleImage*>(Rune Lillesveen . resolved
Daniil SakhapovSo ContentData actually needs to be mutable? Or is this a case of "let's stop here because it requires changes everywhere"?
The let's stop here case for my CL, but it seems like some image resource observer also abuses content mutability.
But to fix that sounds like a separate CL?
Attention is currently required from:
- Daniil Sakhapov
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Dec 4, 2025, 9:51:00 AM (6 days ago) Dec 4
to Menard, Alexis, (Julie)Jeongeun Kim, Kevin Babbitt, Rune Lillesveen, Chromium LUCI CQ, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, zol...@webkit.org, abigailbk...@google.com, blink-rev...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, josiah...@chromium.org, kyungjunle...@google.com, lucasrada...@google.com, nektar...@chromium.org, yuzo+...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Rune Lillesveen
Daniil Sakhapov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
1 comment
File third_party/blink/renderer/core/dom/pseudo_element.cc
Line 413, Patchset 3:
builder.SetContent(const_cast<ContentData*>(style.GetContentData()));Rune Lillesveen . resolved
Daniil SakhapovDoes SetContent() need a mutable ContentData?
Rune LillesveenSee the other comment + I couldn't easily change css_properties.json5 to make content immutable.
Can be a separate CL?
Yes. Then I'd prefer to remove the constness of the ContentData returned from ComputedStyle. I think that's better than having const_casts.
Attention is currently required from:
- Rune Lillesveen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Dec 5, 2025, 4:52:22 AM (5 days ago) Dec 5
to Daniil Sakhapov, Rune Lillesveen, Menard, Alexis, (Julie)Jeongeun Kim, Kevin Babbitt, Chromium LUCI CQ, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, zol...@webkit.org, abigailbk...@google.com, blink-rev...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, josiah...@chromium.org, kyungjunle...@google.com, lucasrada...@google.com, nektar...@chromium.org, yuzo+...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Daniil Sakhapov
Rune Lillesveen voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rune Lillesveen
| Code-Review | +1 |
2 comments
Patchset-level comments
File third_party/blink/renderer/modules/accessibility/ax_node_object.cc
Line 1518, Patchset 5 (Latest):
if (element->IsPseudoElement()) {Rune Lillesveen . unresolved
Optional nit: could've used DynamicTo<> here and avoided the To<> below.
Related details
Attention is currently required from:
- Daniil Sakhapov
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Dec 5, 2025, 7:09:36 AM (5 days ago) Dec 5
to Rune Lillesveen, Menard, Alexis, (Julie)Jeongeun Kim, Kevin Babbitt, Chromium LUCI CQ, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, zol...@webkit.org, abigailbk...@google.com, blink-rev...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, josiah...@chromium.org, kyungjunle...@google.com, lucasrada...@google.com, nektar...@chromium.org, yuzo+...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Rune Lillesveen
Daniil Sakhapov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
1 comment
File third_party/blink/renderer/modules/accessibility/ax_node_object.cc
Optional nit: could've used DynamicTo<> here and avoided the To<> below.
Attention is currently required from:
- Rune Lillesveen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Dec 5, 2025, 7:44:04 AM (5 days ago) Dec 5
to Daniil Sakhapov, Rune Lillesveen, Menard, Alexis, (Julie)Jeongeun Kim, Kevin Babbitt, Chromium LUCI CQ, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, zol...@webkit.org, abigailbk...@google.com, blink-rev...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, josiah...@chromium.org, kyungjunle...@google.com, lucasrada...@google.com, nektar...@chromium.org, yuzo+...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Attention needed from Daniil Sakhapov
Rune Lillesveen voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Daniil Sakhapov
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniil Sakhapov (Gerrit)
Dec 5, 2025, 8:46:49 AM (5 days ago) Dec 5
to Rune Lillesveen, Menard, Alexis, (Julie)Jeongeun Kim, Kevin Babbitt, Chromium LUCI CQ, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, zol...@webkit.org, abigailbk...@google.com, blink-rev...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, josiah...@chromium.org, kyungjunle...@google.com, lucasrada...@google.com, nektar...@chromium.org, yuzo+...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Daniil Sakhapov voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Dec 5, 2025, 8:51:42 AM (5 days ago) Dec 5
to Daniil Sakhapov, Rune Lillesveen, Menard, Alexis, Akihiro Ota, (Julie)Jeongeun Kim, Kevin Babbitt, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, zol...@webkit.org, abigailbk...@google.com, blink-rev...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, josiah...@chromium.org, kyungjunle...@google.com, lucasrada...@google.com, nektar...@chromium.org, yuzo+...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Fix counter() and counters() in alt content
Before that, the value of the counter text were saved in content data,
but content data was shared between the same pseudo-elements. This CL
clones content data so that each pseudo-element has its own copy.
Also, this CL only updates the counter text when attaching layout tree
from the root, as we do for regular counters.
Fixed: 463528045
Change-Id: I564eb5e086a319b77d28338950b305d24db720c2
Reviewed-by: Rune Lillesveen <fut...@chromium.org>
Commit-Queue: Daniil Sakhapov <sakh...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1554610}
Files:
- M third_party/blink/renderer/core/css/style_engine.cc
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/core/dom/element.h
- M third_party/blink/renderer/core/dom/element_rare_data_vector.cc
- M third_party/blink/renderer/core/dom/element_rare_data_vector.h
- M third_party/blink/renderer/core/dom/pseudo_element.cc
- M third_party/blink/renderer/core/dom/pseudo_element.h
- M third_party/blink/renderer/core/style/content_data.cc
- M third_party/blink/renderer/core/style/content_data.h
- M third_party/blink/renderer/modules/accessibility/ax_node_object.cc
- M third_party/blink/web_tests/external/wpt/accname/name/comp_name_from_content.html
- A third_party/blink/web_tests/external/wpt/accname/name/comp_name_from_content_alt_counter_multi_instance.html
Change size: M
Delta: 12 files changed, 206 insertions(+), 37 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Rune Lillesveen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Dec 5, 2025, 9:32:55 AM (5 days ago) Dec 5
to Chromium LUCI CQ, Daniil Sakhapov, Rune Lillesveen, Menard, Alexis, (Julie)Jeongeun Kim, Kevin Babbitt, chromium...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, zol...@webkit.org, abigailbk...@google.com, blink-rev...@chromium.org, dtseng...@chromium.org, francisjp...@google.com, josiah...@chromium.org, kyungjunle...@google.com, lucasrada...@google.com, nektar...@chromium.org, yuzo+...@chromium.org, blink-re...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/56524
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages