Overscroll: Use html attributes to set up overscroll areas [chromium/src : main]
Vladimir Levin (Gerrit)
Vladimir Levin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vladimir Levin
| Commit-Queue | +1 |
1 comment
Related details
- David Baron
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladimir Levin (Gerrit)
Vladimir Levin added 1 comment![]( "Open in Gerrit")
CommandEventType Element::GetCommandEventType(This is largely copied from HTMLElement, with an addition for "ToggleOverscroll"
Related details
- David Baron
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 8 comments![]( "Open in Gerrit")
kPseudoOverscrollTarget,It doesn't look like there's any ordering requirement on this enum, can we move this next to the OverscrollAreaParent pseudo-element?
if (!element.FastHasAttribute(html_names::kIdAttr)) {You can get the attribute and check if it returns g_null_atom to avoid needing to look up the id attribute twice here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/element.h;l=2596;drc=397107e399d4b0f4a6e801e7a888d7d71778529c;bpv=1;bpt=1
// TODO(vmpstr): We can optimize this in some cases since a container thatFile a bug for this optimization? I suspect it can be done without a style update both ways though this is also probably not going to change often.
GetDocument().AddOverscrollCommandTarget(command_for);This looks backwards, I think you meant to delete a command target if the old value was an overscroll command, and add one if the new value is.
target->OverscrollTargetStateChanged();Do we need to notify the old target as well?
I think we should move this notification into the function on document that adds / removes ids from the overscroll command target list.
// TODO(vmpstr): What should we do if there is no overscroll container?Doing nothing seems fine, though I suppose we could consider making the root an implicit overscroll container.
CommandEventType Element::GetCommandEventType(This is largely copied from HTMLElement, with an addition for "ToggleOverscroll"
Why do we need this on Element? I think you could move the change detection code to HTMLElement::AttributeChanged.
Commands are only allowed on a few specific HTMLElement types.
"toggle-overscroll",This should probably be with the rest of the command types.
Related details
- David Baron
- Vladimir Levin
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. |
Vladimir Levin (Gerrit)
Vladimir Levin added 8 comments![]( "Open in Gerrit")
It doesn't look like there's any ordering requirement on this enum, can we move this next to the OverscrollAreaParent pseudo-element?
Done
You can get the attribute and check if it returns g_null_atom to avoid needing to look up the id attribute twice here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/element.h;l=2596;drc=397107e399d4b0f4a6e801e7a888d7d71778529c;bpv=1;bpt=1
Done
// TODO(vmpstr): We can optimize this in some cases since a container thatFile a bug for this optimization? I suspect it can be done without a style update both ways though this is also probably not going to change often.
Done
GetDocument().AddOverscrollCommandTarget(command_for);This looks backwards, I think you meant to delete a command target if the old value was an overscroll command, and add one if the new value is.
Done
Do we need to notify the old target as well?
I think we should move this notification into the function on document that adds / removes ids from the overscroll command target list.
Done.
// TODO(vmpstr): What should we do if there is no overscroll container?Doing nothing seems fine, though I suppose we could consider making the root an implicit overscroll container.
Done
Robert FlackThis is largely copied from HTMLElement, with an addition for "ToggleOverscroll"
Why do we need this on Element? I think you could move the change detection code to HTMLElement::AttributeChanged.
Commands are only allowed on a few specific HTMLElement types.
Yeah, it was for consistency with the Id changes that are handled on element. Also CommandEventType itself is defined on Element and has various things on it. I made this function static as well since it doesn't actually matter where this is called.
Let me know if you want me to move it back and move command parsing there I guess and keep id parsing here.
This should probably be with the rest of the command types.
- David Baron
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
CommandEventType Element::GetCommandEventType(Robert FlackThis is largely copied from HTMLElement, with an addition for "ToggleOverscroll"
Vladimir LevinWhy do we need this on Element? I think you could move the change detection code to HTMLElement::AttributeChanged.
Commands are only allowed on a few specific HTMLElement types.
Yeah, it was for consistency with the Id changes that are handled on element. Also CommandEventType itself is defined on Element and has various things on it. I made this function static as well since it doesn't actually matter where this is called.
Let me know if you want me to move it back and move command parsing there I guess and keep id parsing here.
I think it would be cleaner to split up, since HTMLElement is where most of the command related code is and command and commandfor are only valid on HTMLElement.
Related details
- David Baron
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. |
Vladimir Levin (Gerrit)
Vladimir Levin added 1 comment![]( "Open in Gerrit")
CommandEventType Element::GetCommandEventType(Robert FlackThis is largely copied from HTMLElement, with an addition for "ToggleOverscroll"
Vladimir LevinWhy do we need this on Element? I think you could move the change detection code to HTMLElement::AttributeChanged.
Commands are only allowed on a few specific HTMLElement types.
Robert FlackYeah, it was for consistency with the Id changes that are handled on element. Also CommandEventType itself is defined on Element and has various things on it. I made this function static as well since it doesn't actually matter where this is called.
Let me know if you want me to move it back and move command parsing there I guess and keep id parsing here.
I think it would be cleaner to split up, since HTMLElement is where most of the command related code is and command and commandfor are only valid on HTMLElement.
- David Baron
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack voted and added 3 comments![]( "Open in Gerrit")
Votes added by Robert Flack
| Code-Review | +1 |
3 comments
Element* overscroll_container = nullptr;nit: Move this before the bools so that the structure can pack more efficiently.
HashSet<AtomicString> overscroll_command_targets_;nit: Document what these are, and the resulting pseudo-class on those identified elements.
if (RuntimeEnabledFeatures::CSSOverscrollGesturesEnabled()) {Since GetCommandEventType will only return kToggleOverscroll if the flag is enabled, maybe we don't need this check?
Related details
- David Baron
- Vladimir Levin
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. |
Vladimir Levin (Gerrit)
Vladimir Levin added 3 comments![]( "Open in Gerrit")
nit: Move this before the bools so that the structure can pack more efficiently.
Done
nit: Document what these are, and the resulting pseudo-class on those identified elements.
Done
if (RuntimeEnabledFeatures::CSSOverscrollGesturesEnabled()) {Since GetCommandEventType will only return kToggleOverscroll if the flag is enabled, maybe we don't need this check?
Done
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 7 comments![]( "Open in Gerrit")
Sorry for the delay getting to this. A few comments and questions below.
name: "-internal-overscroll-area",Is it useful to have a comment somewhere explaining how the `-internal-overscroll-area` and `-internal-overscroll-position` properties related to the `overscroll-area` and `overscroll-position` properties?
const AtomicString& id = element.FastGetAttribute(html_names::kIdAttr);I'm a little surprised that this is matched *only* by being referenced by a command elsewhere -- meaning I'm a little surprised that you don't need anything on the element itself. Is that the intent?
if (auto* element = getElementById(target)) {I wonder if this should support [referencetarget](https://github.com/whatwg/html/issues/10707) and thus use `Element::GetElementAttributeResolvingReferenceTarget`... I'm not sure whether it makes sense or not.
SetNeedsStyleRecalc(kLocalStyleChange,I don't think this `SetNeedStyleRecalc` should be needed. The `PseudoStateChanged` should be sufficient if the style side of things is implemented correctly -- and if it's not implemented correctly then the `SetNeedsStyleRecalc` is not sufficient since it won't handle the cases where selector combinators are involved (although you probably don't really have any since it's internal).
Needing it might be a sign of not having updated `InvalidationSetForSimpleSelector`... which you didn't do!
(`PatchStateChanged` might have the same issue.)
if (old_is_overscroll != new_is_overscroll && !command_for.IsNull()) {Do you really want to allow empty string here? (Maybe there's some standard code for validating something is a single name?)
(I guess it doesn't seem like other uses validate much, but that seems a little odd...)
// TODO(crbug.com/463970475): Implement.I think there is a class somewhere that implements this sort of lazy sorting of elements...
Related details
- Vladimir Levin
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. |
Vladimir Levin (Gerrit)
Vladimir Levin added 6 comments![]( "Open in Gerrit")
name: "-internal-overscroll-area",Is it useful to have a comment somewhere explaining how the `-internal-overscroll-area` and `-internal-overscroll-position` properties related to the `overscroll-area` and `overscroll-position` properties?
That's the neat part, they don't!
The latter ones are going away in a clean up patch that's going to be written fairly soon
const AtomicString& id = element.FastGetAttribute(html_names::kIdAttr);I'm a little surprised that this is matched *only* by being referenced by a command elsewhere -- meaning I'm a little surprised that you don't need anything on the element itself. Is that the intent?
Yes it is. The idea is that the button defines the connection of the overscrollcontainer and any other element without needing to annotate it (other than it having an id of course)
if (auto* element = getElementById(target)) {I wonder if this should support [referencetarget](https://github.com/whatwg/html/issues/10707) and thus use `Element::GetElementAttributeResolvingReferenceTarget`... I'm not sure whether it makes sense or not.
Yeah I'm not sure either... It seems like if you have a referencetarget (and I'm only learning about this today), the intent is to actually put the custom element into the overscroll instead of some of its shadow descendants.. We can maybe revisit in a discussion
SetNeedsStyleRecalc(kLocalStyleChange,I don't think this `SetNeedStyleRecalc` should be needed. The `PseudoStateChanged` should be sufficient if the style side of things is implemented correctly -- and if it's not implemented correctly then the `SetNeedsStyleRecalc` is not sufficient since it won't handle the cases where selector combinators are involved (although you probably don't really have any since it's internal).
Needing it might be a sign of not having updated `InvalidationSetForSimpleSelector`... which you didn't do!
(`PatchStateChanged` might have the same issue.)
It seems like also active view transition, and focus visible and focus state all follow the same pattern. Would you accept an argument from consistency with the rest of the code? 😊
We can maybe update them all at once in a separate CL
if (old_is_overscroll != new_is_overscroll && !command_for.IsNull()) {Do you really want to allow empty string here? (Maybe there's some standard code for validating something is a single name?)
(I guess it doesn't seem like other uses validate much, but that seems a little odd...)
You mean why am I not checking command_for is ""? I'm not sure if that's a valid name, I guess not? like if commandfor="", and there's an id="" I would be very surprised if those match
// TODO(crbug.com/463970475): Implement.I think there is a class somewhere that implements this sort of lazy sorting of elements...
Yep, already added in the next patch: https://chromium-review.googlesource.com/c/chromium/src/+/7256355/2
Related details
- David Baron
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. |