Match ::placeholder and ::file-selector-button for getComputedStyle on any element [chromium/src : main]
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 3 comments![]( "Open in Gerrit")
(pseudo_style_request_.pseudo_id == kPseudoIdPlaceholder ||
pseudo_style_request_.pseudo_id == kPseudoIdFileSelectorButton)) {maybe also extract this into a separate function?
// For getComputedStyle, match the pseudo-element selector even if
// the actual shadow element doesn't exist on this element type.
if (context.for_computed_style &&
context.pseudo_id == kPseudoIdFileSelectorButton) {
result.dynamic_pseudo = kPseudoIdFileSelectorButton;
return true;
}I wonder if we can pull this pattern (from kPseudoFileSelectorButton and kPseudoPlaceholder) into some function and check before this switch? And just add new such pseudo-elements there if needed in the future?
result.dynamic_pseudo = context.pseudo_id;I wonder if it did work before without setting dynamic_pseudo? Does it break something if we don't add it here? (just wondering, as this part of the code hasn't been really finished)
I suspect that DynamicPseudoExtractor is doing this dynamic_pseudo line here instead.
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
(pseudo_style_request_.pseudo_id == kPseudoIdPlaceholder ||
pseudo_style_request_.pseudo_id == kPseudoIdFileSelectorButton)) {maybe also extract this into a separate function?
Done
// For getComputedStyle, match the pseudo-element selector even if
// the actual shadow element doesn't exist on this element type.
if (context.for_computed_style &&
context.pseudo_id == kPseudoIdFileSelectorButton) {
result.dynamic_pseudo = kPseudoIdFileSelectorButton;
return true;
}I wonder if we can pull this pattern (from kPseudoFileSelectorButton and kPseudoPlaceholder) into some function and check before this switch? And just add new such pseudo-elements there if needed in the future?
Done
result.dynamic_pseudo = context.pseudo_id;I wonder if it did work before without setting dynamic_pseudo? Does it break something if we don't add it here? (just wondering, as this part of the code hasn't been really finished)
I suspect that DynamicPseudoExtractor is doing this dynamic_pseudo line here instead.
I tried dropping the dynamic_pseudo assignment here, but it’s safer to keep it setting dynamic_pseudo ensures ElementRuleCollector treats the match as a pseudo-element match rather than attributing it to the originating element.
Even if DynamicPseudoExtractor sets it in other flows, we still need it here for consistency.
Related details
- Daniil Sakhapov
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)
Daniil Sakhapov added 1 comment![]( "Open in Gerrit")
result.dynamic_pseudo = context.pseudo_id;Helmut JanuschkaI wonder if it did work before without setting dynamic_pseudo? Does it break something if we don't add it here? (just wondering, as this part of the code hasn't been really finished)
I suspect that DynamicPseudoExtractor is doing this dynamic_pseudo line here instead.
I tried dropping the dynamic_pseudo assignment here, but it’s safer to keep it setting dynamic_pseudo ensures ElementRuleCollector treats the match as a pseudo-element match rather than attributing it to the originating element.
Even if DynamicPseudoExtractor sets it in other flows, we still need it here for consistency.
CheckVirtualPseudo has been implemented for a new way to match pseudo-elements, it really doesn't need setting dynamic_pseudo here, as the matching order for selector is changed, so it can even be bad to set it here tbh.
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
result.dynamic_pseudo = context.pseudo_id;Helmut JanuschkaI wonder if it did work before without setting dynamic_pseudo? Does it break something if we don't add it here? (just wondering, as this part of the code hasn't been really finished)
I suspect that DynamicPseudoExtractor is doing this dynamic_pseudo line here instead.
Daniil SakhapovI tried dropping the dynamic_pseudo assignment here, but it’s safer to keep it setting dynamic_pseudo ensures ElementRuleCollector treats the match as a pseudo-element match rather than attributing it to the originating element.
Even if DynamicPseudoExtractor sets it in other flows, we still need it here for consistency.
CheckVirtualPseudo has been implemented for a new way to match pseudo-elements, it really doesn't need setting dynamic_pseudo here, as the matching order for selector is changed, so it can even be bad to set it here tbh.
removed it, still works! thanks for pushing back! must have been some other change that lead me to the believe tests fail with the dynamic_pseudo force!
Related details
- Daniil Sakhapov
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)
Daniil Sakhapov added 3 comments![]( "Open in Gerrit")
if (CollectRequestedPseudoRulesForComputedStyle<stop_at_first_match>(nit: having this return `true` only for stop_at_first_match is a bit confusing, don't you think? Like if `false` means that nothing has been collected?
When I asked to pull this part into a separate function I meant only pseudo_id if, so that we can add new such pseudo-elements just to that function.
Maybe keep the for loop here as you did originally and just have that if function?
result.dynamic_pseudo = context.pseudo_id;Helmut JanuschkaI wonder if it did work before without setting dynamic_pseudo? Does it break something if we don't add it here? (just wondering, as this part of the code hasn't been really finished)
I suspect that DynamicPseudoExtractor is doing this dynamic_pseudo line here instead.
Daniil SakhapovI tried dropping the dynamic_pseudo assignment here, but it’s safer to keep it setting dynamic_pseudo ensures ElementRuleCollector treats the match as a pseudo-element match rather than attributing it to the originating element.
Even if DynamicPseudoExtractor sets it in other flows, we still need it here for consistency.
Helmut JanuschkaCheckVirtualPseudo has been implemented for a new way to match pseudo-elements, it really doesn't need setting dynamic_pseudo here, as the matching order for selector is changed, so it can even be bad to set it here tbh.
removed it, still works! thanks for pushing back! must have been some other change that lead me to the believe tests fail with the dynamic_pseudo force!
no problem! but that flag is disabled now, so I wonder if we need to create a virtual test to also cover this cide path?
though Gerrit says it's covered by tests...
if (context.pseudo_id ==
selector.GetPseudoId(selector.GetPseudoType())) {
return true;
}
return false;nit: leave as it used to be? (single return)
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
if (CollectRequestedPseudoRulesForComputedStyle<stop_at_first_match>(nit: having this return `true` only for stop_at_first_match is a bit confusing, don't you think? Like if `false` means that nothing has been collected?
When I asked to pull this part into a separate function I meant only pseudo_id if, so that we can add new such pseudo-elements just to that function.
Maybe keep the for loop here as you did originally and just have that if function?
Done. extracted the pseudo_id check into a helper and made the stop_at_first_match return explicit while keeping the loop here.
result.dynamic_pseudo = context.pseudo_id;Helmut JanuschkaI wonder if it did work before without setting dynamic_pseudo? Does it break something if we don't add it here? (just wondering, as this part of the code hasn't been really finished)
I suspect that DynamicPseudoExtractor is doing this dynamic_pseudo line here instead.
Daniil SakhapovI tried dropping the dynamic_pseudo assignment here, but it’s safer to keep it setting dynamic_pseudo ensures ElementRuleCollector treats the match as a pseudo-element match rather than attributing it to the originating element.
Even if DynamicPseudoExtractor sets it in other flows, we still need it here for consistency.
Helmut JanuschkaCheckVirtualPseudo has been implemented for a new way to match pseudo-elements, it really doesn't need setting dynamic_pseudo here, as the matching order for selector is changed, so it can even be bad to set it here tbh.
Daniil Sakhapovremoved it, still works! thanks for pushing back! must have been some other change that lead me to the believe tests fail with the dynamic_pseudo force!
no problem! but that flag is disabled now, so I wonder if we need to create a virtual test to also cover this cide path?
though Gerrit says it's covered by tests...
Good point. I added a virtualsuite to cover this code path and confirmed it passes with CSSLogicalCombinationPseudo enabled.
While testing, figured out i still need `result.dynamic_pseudo = context.pseudo_id` in `CheckVirtualPseudo` without it, the virtual test fails (computed ::before styles do not apply).
if (context.pseudo_id ==
selector.GetPseudoId(selector.GetPseudoType())) {
return true;
}
return false;nit: leave as it used to be? (single return)
- Daniil Sakhapov
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)
Daniil Sakhapov voted Code-Review+1![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |