[a11y][carousel] Remove content from inactive column tabs from AX tree [chromium/src : main]
Morten Stenshorne (Gerrit)
Morten Stenshorne added 4 comments![]( "Open in Gerrit")
// Only process the first fragment for each node to avoid duplicate work
// for fragmented elements.
if (is_first_for_node) {I don't get this. If a node is fragmented, it should be considered active as long as one fragment is in the active column, no?
return DynamicTo<ColumnPseudoElement>(scroll_marker_pseudo->parentElement());Can this ever be anything else than ColumnPseudoElement?
Maybe To<> instead of DynamicTo<>?
This seems fine in itself (except for my remark below), but I just started wondering: how do we deal with relayouts? What used to be in the active column may no longer be, and vice versa, if things move around.
Maybe we actually need to update the LayoutObject bit during layout, if there's an active column. In addition to what's here. But we can follow up with a CL to handle that, and not worry about it here.
if (!old_column) {This part needs to be done before `new_column->SetIsInsideInactiveColumnTabForDescendants(false)`, in case some element exists both in the old and the new column (then the element should be considered to be in an active column, right?).
So... above:
```
if (old_column) {
// Mark all content in the old active column as inactive.
old_column->SetIsInsideInactiveColumnTabForDescendants(true);
} else {
[this thing]
}
if (new_column) {
// Mark all content in the new active column as active.
new_column->SetIsInsideInactiveColumnTabForDescendants(false);
}
```
Related details
- Daniil Sakhapov
- Robert Flack
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. |
Robert Flack (Gerrit)
Robert Flack added 2 comments![]( "Open in Gerrit")
// Only process the first fragment for each node to avoid duplicate work
// for fragmented elements.
if (is_first_for_node) {I don't get this. If a node is fragmented, it should be considered active as long as one fragment is in the active column, no?
Yes, this is correct, any fragment being in the active column should count.
return DynamicTo<ColumnPseudoElement>(scroll_marker_pseudo->parentElement());Can this ever be anything else than ColumnPseudoElement?
Maybe To<> instead of DynamicTo<>?
Scroll markers can originate from elements (https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/::scroll-marker) or columns (https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/::column).
Related details
- Daniil Sakhapov
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
return DynamicTo<ColumnPseudoElement>(scroll_marker_pseudo->parentElement());Robert FlackCan this ever be anything else than ColumnPseudoElement?
Maybe To<> instead of DynamicTo<>?
Scroll markers can originate from elements (https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/::scroll-marker) or columns (https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/::column).
Yes, for some reason I assumed that we had to be in multicol here. But I guess not.
Related details
- Daniil Sakhapov
- Robert Flack
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 4 comments![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
4 comments
Daniil SakhapovOverall the expectation looks good. I think there may be better ways to optimize the performance - in theory we should only have to walk the column fragments once where it looks like right now it looks like this will walk the fragment descendents again for each non-excluded descendant until it finds that descendant.
Robert Flackagree, fixed to compute ignore state only on "roots".
Robert FlackFor some reason i'm unable to `git cl patch` this in to my local tree to test. Does this work if there is an element between the column establishing element and the inner elements? E.g. this is a modified version of your test case:
```html
<!doctype html><style>
.carousel {
height: 200px;
width: 200px;
columns: 1;
overflow-x: auto;
overscroll-behavior-x: contain;
scroll-snap-type: x mandatory;
scroll-marker-group: after tabs;&::scroll-marker-group {
height: 1rem;
width: 100%;
text-align: center;
}&::column {
scroll-snap-align: center;
}&::column::scroll-marker {
display: inline-block;
content: '';
width: 1rem;
height: 1rem;
border: 1px solid black;
}&::column::scroll-marker:target-current {
background: blue;
}& div {
position: relative;
height: 100%;
}
& div div {
display: inline-flex;
width: 30%;
height: 100%;
}
}
</style><div id="scroller" class=carousel>
<div>
<div>One</div>
<div>Two</div>
<div>Three</div>
<div>Four</div>
<div>Five</div>
<div>Six</div>
</div>
</div>
```My thinking was that we might need to set something on all of the visited nodes in a walk or save the visited node in a hashset in order to only require a single walk of the tree.
Daniil SakhapovThat said, I'm not sure how common this kind of setup is.
Robert FlackHmm, it doesn't work indeed.
@mste...@chromium.org active_column->Contains() returns true for the wrapping div from the test, is my impl of Contains that's wrong or the wrapping div is indeed inside ::column?
also, I don't really understand how this setup is supposed to work?
Morten StenshorneRight, I expect the wrapping div would be contained, but that not all of its children would be. This is why I'm thinking we may need to do a walk at least as deep as the first non-fragmented children.
Daniil SakhapovThe wrapping DIV is contained by every column. The inner DIVs are atomic inlines and each of them belong only in one of the columns. Nesting like this shouldn't be unusual at all. Sorry, what doesn't work?
The ColumnPseudoElement code looks good now. I don't / didn't know how the accessibility code works, but taking a look at it now, if AXObject::UpdateCachedAttributeValuesIfNeeded() is called for every node in the tree, it does seem problematic (O(n^2)) to do so much work for every node, i.e. locate the active column and check that the node isn't in there.
So maybe we'd be better off with a different approach. When changing the active column, we could mark everything inside the now-inactive column as inactive, and then mark everything inside the now-active column as active (by "marking", I probably mean a bit on LayoutObject or something like that).
Daniil Sakhapovptal
Done
// Only process the first fragment for each node to avoid duplicate work
// for fragmented elements.
if (is_first_for_node) {Robert FlackI don't get this. If a node is fragmented, it should be considered active as long as one fragment is in the active column, no?
Yes, this is correct, any fragment being in the active column should count.
Done
This seems fine in itself (except for my remark below), but I just started wondering: how do we deal with relayouts? What used to be in the active column may no longer be, and vice versa, if things move around.
Maybe we actually need to update the LayoutObject bit during layout, if there's an active column. In addition to what's here. But we can follow up with a CL to handle that, and not worry about it here.
Acknowledged
This part needs to be done before `new_column->SetIsInsideInactiveColumnTabForDescendants(false)`, in case some element exists both in the old and the new column (then the element should be considered to be in an active column, right?).
So... above:
```
if (old_column) {
// Mark all content in the old active column as inactive.
old_column->SetIsInsideInactiveColumnTabForDescendants(true);
} else {
[this thing]
}
if (new_column) {
// Mark all content in the new active column as active.
new_column->SetIsInsideInactiveColumnTabForDescendants(false);
}
```
- Morten Stenshorne
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 2 comments![]( "Open in Gerrit")
if (is_first_for_node || !is_inactive_) {What breaks if we remove this check?
Being first or not should really not make a difference.
} else if (new_column) {Always true. Change to DCHECK.
Related details
- Daniil Sakhapov
- Robert Flack
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")
if (is_first_for_node || !is_inactive_) {What breaks if we remove this check?
Being first or not should really not make a difference.
- Morten Stenshorne
- Robert Flack
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
if (is_first_for_node || !is_inactive_) {Daniil SakhapovWhat breaks if we remove this check?
Being first or not should really not make a difference.
just as a micro-optimization? xD
better to remove?
- Daniil Sakhapov
- Robert Flack
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
2 comments
Daniil SakhapovWhat breaks if we remove this check?
Being first or not should really not make a difference.
Morten Stenshornejust as a micro-optimization? xD
better to remove?
Yes, it would probably cause bugs.
Done
Always true. Change to DCHECK.
- Morten Stenshorne
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 2 comments![]( "Open in Gerrit")
return true;Is this still needed, and what does it have to do with columns?
Related details
- Daniil Sakhapov
- Robert Flack
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 2 comments![]( "Open in Gerrit")
Is everything ready for review?
👍
return true;Is this still needed, and what does it have to do with columns?
This function is to handle "am I inside an originating element for inactive ::scroll-marker " case. Nothing to do with columns.
Hence, in this case I can just inherit that flag from the parent.
Related details
- Morten Stenshorne
- Robert Flack
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
return true;Daniil SakhapovIs this still needed, and what does it have to do with columns?
This function is to handle "am I inside an originating element for inactive ::scroll-marker " case. Nothing to do with columns.
Hence, in this case I can just inherit that flag from the parent.
Isn't this CL about something column-specific, though?
Maybe do it in a different CL. I can't see any tests for it, either.
Related details
- Daniil Sakhapov
- Robert Flack
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 1 comment![]( "Open in Gerrit")
return true;Daniil SakhapovIs this still needed, and what does it have to do with columns?
Morten StenshorneThis function is to handle "am I inside an originating element for inactive ::scroll-marker " case. Nothing to do with columns.
Hence, in this case I can just inherit that flag from the parent.
Isn't this CL about something column-specific, though?
Maybe do it in a different CL. I can't see any tests for it, either.
it's just a small fix that is covered by content/test/data/accessibility/css/carousel-with-column-tabs-expected-blink.txt
I'd like to keep it, if that's OK?
Related details
- Morten Stenshorne
- Robert Flack
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
return true;Daniil SakhapovIs this still needed, and what does it have to do with columns?
Morten StenshorneThis function is to handle "am I inside an originating element for inactive ::scroll-marker " case. Nothing to do with columns.
Hence, in this case I can just inherit that flag from the parent.
Daniil SakhapovIsn't this CL about something column-specific, though?
Maybe do it in a different CL. I can't see any tests for it, either.
it's just a small fix that is covered by content/test/data/accessibility/css/carousel-with-column-tabs-expected-blink.txt
I'd like to keep it, if that's OK?
Only if there are inter-dependencies between that and the rest.
Small fix, small CL?
Related details
- Daniil Sakhapov
- Robert Flack
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 1 comment![]( "Open in Gerrit")
return true;Daniil SakhapovIs this still needed, and what does it have to do with columns?
Morten StenshorneThis function is to handle "am I inside an originating element for inactive ::scroll-marker " case. Nothing to do with columns.
Hence, in this case I can just inherit that flag from the parent.
Daniil SakhapovIsn't this CL about something column-specific, though?
Maybe do it in a different CL. I can't see any tests for it, either.
Morten Stenshorneit's just a small fix that is covered by content/test/data/accessibility/css/carousel-with-column-tabs-expected-blink.txt
I'd like to keep it, if that's OK?
Only if there are inter-dependencies between that and the rest.
Small fix, small CL?
I've introduced IsDescendantOfTabsModeScroller in this CL and changed a couple of names here and there.
But I guess it can be split out, if much needed.
Related details
- Morten Stenshorne
- Robert Flack
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
return true;Daniil SakhapovIs this still needed, and what does it have to do with columns?
Morten StenshorneThis function is to handle "am I inside an originating element for inactive ::scroll-marker " case. Nothing to do with columns.
Hence, in this case I can just inherit that flag from the parent.
Daniil SakhapovIsn't this CL about something column-specific, though?
Maybe do it in a different CL. I can't see any tests for it, either.
Morten Stenshorneit's just a small fix that is covered by content/test/data/accessibility/css/carousel-with-column-tabs-expected-blink.txt
I'd like to keep it, if that's OK?
Daniil SakhapovOnly if there are inter-dependencies between that and the rest.
Small fix, small CL?
I've introduced IsDescendantOfTabsModeScroller in this CL and changed a couple of names here and there.
But I guess it can be split out, if much needed.
Yes please. Let this CL be about "Remove content from inactive column tabs from AX tree". Much cleaner.
Related details
- Daniil Sakhapov
- Robert Flack
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
1 comment
Daniil SakhapovIs this still needed, and what does it have to do with columns?
Morten StenshorneThis function is to handle "am I inside an originating element for inactive ::scroll-marker " case. Nothing to do with columns.
Hence, in this case I can just inherit that flag from the parent.
Daniil SakhapovIsn't this CL about something column-specific, though?
Maybe do it in a different CL. I can't see any tests for it, either.
Morten Stenshorneit's just a small fix that is covered by content/test/data/accessibility/css/carousel-with-column-tabs-expected-blink.txt
I'd like to keep it, if that's OK?
Daniil SakhapovOnly if there are inter-dependencies between that and the rest.
Small fix, small CL?
Morten StenshorneI've introduced IsDescendantOfTabsModeScroller in this CL and changed a couple of names here and there.
But I guess it can be split out, if much needed.
Yes please. Let this CL be about "Remove content from inactive column tabs from AX tree". Much cleaner.
- Morten Stenshorne
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne voted and added 1 comment![]( "Open in Gerrit")
Votes added by Morten Stenshorne
| Code-Review | +1 |
1 comment
if (!layout_object->IsBox()) {
return ParentObject() && ParentObject()->InsideInactiveColumnTab();
}
// The IsInsideInactiveColumnTab bit is set by ScrollMarkerGroupData when
// the active column changes.
return layout_object->InsideInactiveColumnTab();```suggestion
// The IsInsideInactiveColumnTab bit is set by ScrollMarkerGroupData when
// the active column changes.
return layout_object->EnclosingBox()->InsideInactiveColumnTab();
```
Isn't this what we discussed?
Related details
- Daniil Sakhapov
- Robert Flack
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. |
Robert Flack (Gerrit)
Robert Flack added 3 comments![]( "Open in Gerrit")
if (old_column != new_column) {What if the layout of the content has changed even if we haven't changed active columns. E.g. if there are currently 3 items on the active column but the first one becomes wide and causes the other 2 to move off to the next column? Here's a demo where you can see it - click on Four to make it grow wide:
& .wide {
width: 100%;
}
}
</style><div id="scroller" class=carousel>
<div>
<div>One</div>
<div>Two</div>
<div>Three</div>
<div>Four</div>
<div>Five</div>
<div>Six</div>
</div>
</div>
<script>
document.getElementById('scroller').addEventListener('click', (evt) => {
const target = evt.target;
if (target.parentElement.parentElement.id != 'scroller') return;
target.classList.toggle('wide');
});
</script>
```
In a similar manner, this could also happen if the browser size changes. I think to whatever extent this is cached it will need to be updated when layout changes.
ADD_BOOLEAN_BITFIELD(inside_inactive_column_tab_, InsideInactiveColumnTab);I suspect that it may be cleaner to collect the visited objects from a walk of the active column into a HeapHashSet<WeakMember<LayoutObject>> rather than storing an extra bit on every layoutobject (and would only require walking the active column), however I defer to @mste...@chromium.org whether it's worth doing or just having a bit on the objects.
bool cached_is_ignored_as_inside_inactive_column_tab_ : 1 = false;It seems like the ax object is also caching this state, maybe we don't need a cache on the LayoutObject and could just directly set this on the ax objects?
Related details
- Daniil Sakhapov
Morten Stenshorne (Gerrit)
Morten Stenshorne added 3 comments![]( "Open in Gerrit")
I have already pointed this out, and I suggested that we follow up on it, rather than dealing with it in this CL.
ADD_BOOLEAN_BITFIELD(inside_inactive_column_tab_, InsideInactiveColumnTab);I suspect that it may be cleaner to collect the visited objects from a walk of the active column into a HeapHashSet<WeakMember<LayoutObject>> rather than storing an extra bit on every layoutobject (and would only require walking the active column), however I defer to @mste...@chromium.org whether it's worth doing or just having a bit on the objects.
I think the current solution is fine. Also not sure how collecting into a hash set would be workable when updating for layout, since we may hit the (layout) cache and skip subtrees then.
bool cached_is_ignored_as_inside_inactive_column_tab_ : 1 = false;It seems like the ax object is also caching this state, maybe we don't need a cache on the LayoutObject and could just directly set this on the ax objects?
If we want to update the state during layout (if things move around, and what's in the active tab no longer is, or vice versa), I think layout is the place to store it.
Daniil Sakhapov (Gerrit)
Daniil Sakhapov voted and added 4 comments![]( "Open in Gerrit")
Votes added by Daniil Sakhapov
| Commit-Queue | +1 |
4 comments
Done
ADD_BOOLEAN_BITFIELD(inside_inactive_column_tab_, InsideInactiveColumnTab);Morten StenshorneI suspect that it may be cleaner to collect the visited objects from a walk of the active column into a HeapHashSet<WeakMember<LayoutObject>> rather than storing an extra bit on every layoutobject (and would only require walking the active column), however I defer to @mste...@chromium.org whether it's worth doing or just having a bit on the objects.
I think the current solution is fine. Also not sure how collecting into a hash set would be workable when updating for layout, since we may hit the (layout) cache and skip subtrees then.
Done
if (!layout_object->IsBox()) {
return ParentObject() && ParentObject()->InsideInactiveColumnTab();
}
// The IsInsideInactiveColumnTab bit is set by ScrollMarkerGroupData when
// the active column changes.
return layout_object->InsideInactiveColumnTab();```suggestion
// The IsInsideInactiveColumnTab bit is set by ScrollMarkerGroupData when
// the active column changes.
return layout_object->EnclosingBox()->InsideInactiveColumnTab();
```Isn't this what we discussed?
yes, but !IsBox check seemed like less work in the moment. Updated to your suggestion now.
bool cached_is_ignored_as_inside_inactive_column_tab_ : 1 = false;Morten StenshorneIt seems like the ax object is also caching this state, maybe we don't need a cache on the LayoutObject and could just directly set this on the ax objects?
If we want to update the state during layout (if things move around, and what's in the active tab no longer is, or vice versa), I think layout is the place to store it.
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne voted and added 1 comment![]( "Open in Gerrit")
Votes added by Morten Stenshorne
| Code-Review | +1 |
1 comment
if (!layout_object->IsBox()) {
return ParentObject() && ParentObject()->InsideInactiveColumnTab();
}
// The IsInsideInactiveColumnTab bit is set by ScrollMarkerGroupData when
// the active column changes.
return layout_object->InsideInactiveColumnTab();Daniil Sakhapov```suggestion
// The IsInsideInactiveColumnTab bit is set by ScrollMarkerGroupData when
// the active column changes.
return layout_object->EnclosingBox()->InsideInactiveColumnTab();
```Isn't this what we discussed?
yes, but !IsBox check seemed like less work in the moment. Updated to your suggestion now.
Good.
Just because the object isn't a box doesn't have to mean that its parent is one (and, if it isn't, then InsideInactiveColumnTab() would be wrong, no?).
<div><span><span><span>text.
(and then some further complications for ::first-letter and so on, which may nest LayoutText objects, I think...)
Related details
- Daniil Sakhapov
- Robert Flack
Daniil Sakhapov (Gerrit)
Daniil Sakhapov added 1 comment![]( "Open in Gerrit")
if (!layout_object->IsBox()) {
return ParentObject() && ParentObject()->InsideInactiveColumnTab();
}
// The IsInsideInactiveColumnTab bit is set by ScrollMarkerGroupData when
// the active column changes.
return layout_object->InsideInactiveColumnTab();Daniil Sakhapov```suggestion
// The IsInsideInactiveColumnTab bit is set by ScrollMarkerGroupData when
// the active column changes.
return layout_object->EnclosingBox()->InsideInactiveColumnTab();
```Isn't this what we discussed?
Morten Stenshorneyes, but !IsBox check seemed like less work in the moment. Updated to your suggestion now.
Good.
Just because the object isn't a box doesn't have to mean that its parent is one (and, if it isn't, then InsideInactiveColumnTab() would be wrong, no?).
<div><span><span><span>text.
(and then some further complications for ::first-letter and so on, which may nest LayoutText objects, I think...)
True, but my code, for non-box objects, was asking its AX parent for that bit, so for your example AX for div would have a correct bit set, all other non-box <span> and text children would just take that correct bit from their AX parent, so no need to go up to the box object for every descendant.
Related details
- Robert Flack
Robert Flack (Gerrit)
Robert Flack voted Code-Review+1![]( "Open in Gerrit")
- Daniil Sakhapov
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
Okay, we can follow up on this.