WIP4: Start of 3 approaches to subtracting scrollbars from vw [chromium/src : main]
David Grogan (Gerrit)
David Grogan added 4 comments![]( "Open in Gerrit")
Hi Gentlemen, I've spoken with each of you in the past about this project. I've tried a few things, but none seem very promising. There are some code comments with some descriptions, problems, and questions about each approach. I could go for some technical guidance on which approach (or combo of approaches!) seems most worthwhile to you.
// Approach 1: Make viewport units respect scrollbars even for properties onApproach 1
// Approach 3: mostly a proof-of-concept that seems very unlikely to beApproach 3
Related details
- Anders Hartvoll Ruud
- Ian Kilpatrick
- Rune Lillesveen
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. |
David Grogan (Gerrit)
Message from David Grogan![]( "Open in Gerrit")
Set Ready For Review
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 4 comments![]( "Open in Gerrit")
Hi Gentlemen, I've spoken with each of you in the past about this project. I've tried a few things, but none seem very promising. There are some code comments with some descriptions, problems, and questions about each approach. I could go for some technical guidance on which approach (or combo of approaches!) seems most worthwhile to you.
I mostly looked at Approach 1; the others seem to require a ComputedStyle to already exist? (I believe that's a non-starter?)
// Issue: PaintLayerScrollableArea::UpdateAfterStyleChange,
// PaintLayerScrollableArea::ComputeScrollbarExistence, and
// LayoutView::CalculateScrollbarModes have _a lot_ of logic about the
// existence of scrollbars that goes way beyond the primitive check for
// state_.StyleBuilder().OverflowX() == EOverflow::kScroll here. But thatEven if we remove all the stuff about conditional scrollbars? (Which I assume is in there---I haven't looked.)
Is it viable to refactor `ComputeScrollbarExistence` into basically:
```
bool ComputeScrollbarExistence(args) {
return ComputeUnconditionalScrollbarExistence(args)
|| ComputeConditionalScrollbarExistence(args);
}
```
?
// Question: Should I salvage this approach by stuffing a temporary barebones
// ComputedStyle object with just the properties that
// PaintLayerScrollableArea::ComputeScrollbarExistence and its friends need on
// to the document, and then (somehow) call the scrollbar-existence logic
// here? Or perhaps instead by duplicating some of that logic here?(You'd think the existence of _unconditional_ scrollbars wouldn't be so hard to determine.)
temporary barebones ComputedStyle object
I did exactly this to solve a similar case of "needs ComputedStyle, but doesn't exist yet" when working on the StyleBuilder project:
You make it sound like a hack, but this just means that we'll now have functions that only accept parameters they _actually care about_ instead of pretending to "depend" on everything. As long as the number of fields is reasonably low, this should be fine.
// Question: Am I wrong in thinking thatIf we _cannot_ determine unconditional scrollbars from computed values alone, I think it's a spec issue?
Related details
- David Grogan
- Ian Kilpatrick
- Rune Lillesveen
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)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
I'll let Anders handle this from the style side.
Related details
- David Grogan
- Ian Kilpatrick
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. |
David Grogan (Gerrit)
David Grogan added 5 comments![]( "Open in Gerrit")
I'm back from OOO. Thanks a bunch for the comments.
Anders Hartvoll RuudHi Gentlemen, I've spoken with each of you in the past about this project. I've tried a few things, but none seem very promising. There are some code comments with some descriptions, problems, and questions about each approach. I could go for some technical guidance on which approach (or combo of approaches!) seems most worthwhile to you.
I mostly looked at Approach 1; the others seem to require a ComputedStyle to already exist? (I believe that's a non-starter?)
non-starter status is unclear. IF viewport units in :root property values need to subtract unconditional scrollbars, then requiring a complete ComputedStyle is kind of a non-starter.
BUT viewport units in :root property values needing to subtract unconditional scrollbars is unclear though. We have, or can easily create, some wiggle room in the spec on this point.
// Issue: PaintLayerScrollableArea::UpdateAfterStyleChange,
// PaintLayerScrollableArea::ComputeScrollbarExistence, and
// LayoutView::CalculateScrollbarModes have _a lot_ of logic about the
// existence of scrollbars that goes way beyond the primitive check for
// state_.StyleBuilder().OverflowX() == EOverflow::kScroll here. But thatEven if we remove all the stuff about conditional scrollbars? (Which I assume is in there---I haven't looked.)
Is it viable to refactor `ComputeScrollbarExistence` into basically:
```
bool ComputeScrollbarExistence(args) {
return ComputeUnconditionalScrollbarExistence(args)
|| ComputeConditionalScrollbarExistence(args);
}
```?
ComputeScrollbarExistence is mostly about unconditional scrollbars already, and is essentially already factored the way you suggest. Its first 80 lines are about unconditional scrollbars, but that includes a call to LayoutView::CalculateScrollbarModes, which is another ~100 lines.
(Assuming that by "conditional" you mean conditional on layout overflow. LMK if I've misinterpreted.)
// Question: Should I salvage this approach by stuffing a temporary barebones
// ComputedStyle object with just the properties that
// PaintLayerScrollableArea::ComputeScrollbarExistence and its friends need on
// to the document, and then (somehow) call the scrollbar-existence logic
// here? Or perhaps instead by duplicating some of that logic here?(You'd think the existence of _unconditional_ scrollbars wouldn't be so hard to determine.)
temporary barebones ComputedStyle object
I did exactly this to solve a similar case of "needs ComputedStyle, but doesn't exist yet" when working on the StyleBuilder project:
You make it sound like a hack, but this just means that we'll now have functions that only accept parameters they _actually care about_ instead of pretending to "depend" on everything. As long as the number of fields is reasonably low, this should be fine.
(You'd think the existence of _unconditional_ scrollbars wouldn't be so hard to determine.)
😊
> temporary barebones ComputedStyle objectI did exactly this to solve a similar case of "needs ComputedStyle, but doesn't exist yet" when working on the StyleBuilder project:
I meant an actual instance of the ComputedStyle class with mostly default values. I suppose you opted for a new class for clarity around what properties have been resolved and are actually cared about?
I estimate ~7-10 properties need to be resolved to determine the presence of unconditional scrollbars. Does that strike you as low enough?
// Question: Am I wrong in thinking thatIf we _cannot_ determine unconditional scrollbars from computed values alone, I think it's a spec issue?
The only impactful spec question is whether viewport units _on root element properties_ should have subtracted unconditional scrollbar sizes.
Aside from that, all the issues I described in this CL are of the implementation-complexity variety, not spec problems.
Related details
- Anders Hartvoll Ruud
- Ian Kilpatrick
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. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 3 comments![]( "Open in Gerrit")
Anders Hartvoll RuudHi Gentlemen, I've spoken with each of you in the past about this project. I've tried a few things, but none seem very promising. There are some code comments with some descriptions, problems, and questions about each approach. I could go for some technical guidance on which approach (or combo of approaches!) seems most worthwhile to you.
David GroganI mostly looked at Approach 1; the others seem to require a ComputedStyle to already exist? (I believe that's a non-starter?)
non-starter status is unclear. IF viewport units in :root property values need to subtract unconditional scrollbars, then requiring a complete ComputedStyle is kind of a non-starter.
BUT viewport units in :root property values needing to subtract unconditional scrollbars is unclear though. We have, or can easily create, some wiggle room in the spec on this point.
Ah, OK, I thought viewport units on the root element being aware of this stuff was a fairly strict requirement already.
In any case, it sounds possible to work out.
// Issue: PaintLayerScrollableArea::UpdateAfterStyleChange,
// PaintLayerScrollableArea::ComputeScrollbarExistence, and
// LayoutView::CalculateScrollbarModes have _a lot_ of logic about the
// existence of scrollbars that goes way beyond the primitive check for
// state_.StyleBuilder().OverflowX() == EOverflow::kScroll here. But thatDavid GroganEven if we remove all the stuff about conditional scrollbars? (Which I assume is in there---I haven't looked.)
Is it viable to refactor `ComputeScrollbarExistence` into basically:
```
bool ComputeScrollbarExistence(args) {
return ComputeUnconditionalScrollbarExistence(args)
|| ComputeConditionalScrollbarExistence(args);
}
```?
ComputeScrollbarExistence is mostly about unconditional scrollbars already, and is essentially already factored the way you suggest. Its first 80 lines are about unconditional scrollbars, but that includes a call to LayoutView::CalculateScrollbarModes, which is another ~100 lines.
(Assuming that by "conditional" you mean conditional on layout overflow. LMK if I've misinterpreted.)
Ack.
(Assuming that by "conditional" you mean conditional on layout overflow. LMK if I've misinterpreted.)
That is indeed what I meant.
// Question: Should I salvage this approach by stuffing a temporary barebones
// ComputedStyle object with just the properties that
// PaintLayerScrollableArea::ComputeScrollbarExistence and its friends need on
// to the document, and then (somehow) call the scrollbar-existence logic
// here? Or perhaps instead by duplicating some of that logic here?David Grogan(You'd think the existence of _unconditional_ scrollbars wouldn't be so hard to determine.)
temporary barebones ComputedStyle object
I did exactly this to solve a similar case of "needs ComputedStyle, but doesn't exist yet" when working on the StyleBuilder project:
You make it sound like a hack, but this just means that we'll now have functions that only accept parameters they _actually care about_ instead of pretending to "depend" on everything. As long as the number of fields is reasonably low, this should be fine.
(You'd think the existence of _unconditional_ scrollbars wouldn't be so hard to determine.)
😊
> temporary barebones ComputedStyle objectI did exactly this to solve a similar case of "needs ComputedStyle, but doesn't exist yet" when working on the StyleBuilder project:
I meant an actual instance of the ComputedStyle class with mostly default values. I suppose you opted for a new class for clarity around what properties have been resolved and are actually cared about?
I estimate ~7-10 properties need to be resolved to determine the presence of unconditional scrollbars. Does that strike you as low enough?
I meant an actual instance of the ComputedStyle class with mostly default values.
Yikes.
I estimate ~7-10 properties [...] Does that strike you as low enough?
I think yes, and then we'll see if I need to regret saying that.
Related details
- David Grogan
- Ian Kilpatrick
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. |
David Grogan (Gerrit)
David Grogan added 2 comments![]( "Open in Gerrit")
+skobes for question below
// Question: Should I salvage this approach by stuffing a temporary barebones
// ComputedStyle object with just the properties that
// PaintLayerScrollableArea::ComputeScrollbarExistence and its friends need on
// to the document, and then (somehow) call the scrollbar-existence logic
// here? Or perhaps instead by duplicating some of that logic here?David Grogan(You'd think the existence of _unconditional_ scrollbars wouldn't be so hard to determine.)
temporary barebones ComputedStyle object
I did exactly this to solve a similar case of "needs ComputedStyle, but doesn't exist yet" when working on the StyleBuilder project:
You make it sound like a hack, but this just means that we'll now have functions that only accept parameters they _actually care about_ instead of pretending to "depend" on everything. As long as the number of fields is reasonably low, this should be fine.
Anders Hartvoll Ruud(You'd think the existence of _unconditional_ scrollbars wouldn't be so hard to determine.)
😊
> temporary barebones ComputedStyle objectI did exactly this to solve a similar case of "needs ComputedStyle, but doesn't exist yet" when working on the StyleBuilder project:
I meant an actual instance of the ComputedStyle class with mostly default values. I suppose you opted for a new class for clarity around what properties have been resolved and are actually cared about?
I estimate ~7-10 properties need to be resolved to determine the presence of unconditional scrollbars. Does that strike you as low enough?
I meant an actual instance of the ComputedStyle class with mostly default values.
Yikes.
I estimate ~7-10 properties [...] Does that strike you as low enough?
I think yes, and then we'll see if I need to regret saying that.
@skobes, See above discussion and code comments for full context, but the approach I'm about to embark on is to
(1) resolve a bunch of properties (namely those that are required for determining unconditional scrollbars) before row 335's `ApplyHighPriority`
(2) collect the results into an object of a new type
(3) Refactor PaintLayerScrollableArea::ComputeScrollbarExistence and LayoutView::CalculateScrollbarModes to accept an object of this new type
(4) Call PaintLayerScrollableArea::ComputeScrollbarExistence (and transitively LayoutView::CalculateScrollbarModes) here to check for scrollbar existence and size
(5) Subtract those scrollbar sizes from the viewport size as appropriate
(6) Proceed with style resolution starting from `ApplyHighPriority` below.
With your scrollbar expert hat on, does this sound like a decent plan to detect scrollbar existence before style is fully resolved? What dragons do you foresee in this approach?
Related details
- Anders Hartvoll Ruud
- Ian Kilpatrick
- Steve Kobes
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. |
Steve Kobes (Gerrit)
Steve Kobes added 1 comment![]( "Open in Gerrit")
Are you keeping the existing call sites of `ComputeScrollbarExistence` in addition to the new one you are adding here?
If a scrollbar is added or removed at a later stage, will we trigger a new style recalc to keep the vw units in sync? How do we avoid cycles in that case?
Related details
- David Grogan
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. |
David Grogan (Gerrit)
David Grogan added 1 comment![]( "Open in Gerrit")
Are you keeping the existing call sites of `ComputeScrollbarExistence` in addition to the new one you are adding here?
I think keep them because here we are only interested in unconditional scrollbars specified on the root element; those are the only scrollbars that the new spec tells us to subtract from the viewport size when calculating viewport units.
If a scrollbar is added or removed at a later stage, will we trigger a new style recalc to keep the vw units in sync? How do we avoid cycles in that case?
I don't think we'd trigger a new style recalc for *later stage* changes. The idea is that this new logic would capture all the instances of the layout-unconditional scrollbars. So any later stage changes would come from layout-conditional scrollbars, which we are not supposed to subtract from the viewport size.
Good questions. I honestly hadn't thought this far yet.
Related details
- Anders Hartvoll Ruud
- Steve Kobes
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. |
Steve Kobes (Gerrit)
Steve Kobes added 1 comment![]( "Open in Gerrit")
Hmm it seems pretty weird for the meaning of vw to be different depending on whether the scrollbar is layout-conditional or layout-unconditional. Is that really required by the spec? Shouldn't we treat all (non-overlay) scrollbars the same?
Related details
- Anders Hartvoll Ruud
- David Grogan
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. |
David Grogan (Gerrit)
David Grogan added 1 comment![]( "Open in Gerrit")
Hmm it seems pretty weird for the meaning of vw to be different depending on whether the scrollbar is layout-conditional or layout-unconditional. Is that really required by the spec?
I'm pretty sure, yes. But please check me! https://drafts.csswg.org/css-values-4/#:~:text=In%20all%20cases%2C%20if%20the%20value%20of
Related details
- Anders Hartvoll Ruud
- Steve Kobes
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. |
Steve Kobes (Gerrit)
Steve Kobes added 1 comment![]( "Open in Gerrit")
Given that requirement, I guess the best implementation strategy is to break ComputeScrollbarExistence into two pieces:
- ComputeUnconditionalScrollbarExistence (runs during style recalc)
- ComputeAutoScrollbarExistence (runs after layout)
I still think this behavior seems surprising for web developers and would like to understand the motivation behind it...
Related details
- Anders Hartvoll Ruud
- David Grogan
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. |
David Grogan (Gerrit)
David Grogan added 1 comment![]( "Open in Gerrit")
I still think this behavior seems surprising for web developers and would like to understand the motivation behind it...
https://github.com/w3c/csswg-drafts/issues/1766#issuecomment-1832472781 -- "for cyclic reasons"
Tab's explanation there is not in response to your exact question, but close. (It also illustrates that this issue has had a long history.)
Related details
- Anders Hartvoll Ruud
- Steve Kobes
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. |
Steve Kobes (Gerrit)
Steve Kobes added 1 comment![]( "Open in Gerrit")
Ok I can see that many smart people have thought about this :)
I hope we can be explicit in the code about distinguishing vw-impacting scrollbars from non-vw-impacting scrollbars, if that distinction must exist. Trying to avoid a situation where ComputeScrollbarExistence is overloaded to do slightly different things at different stages.
Maybe we should we introduce a new struct like "StyleBasedScrollbarData", which can be computed at style recalc time and tells us if we have vw-impacting scrollbars, along with their thickness?
Related details
- Anders Hartvoll Ruud
- David Grogan
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. |
David Grogan (Gerrit)
David Grogan added 1 comment![]( "Open in Gerrit")
Ok I can see that many smart people have thought about this :)
Oh, yes, though pointing that out was not my intention :) I was more trying to say that if we want to change it we'll run into some friction.
I hope we can be explicit in the code about distinguishing vw-impacting scrollbars from non-vw-impacting scrollbars, if that distinction must exist. Trying to avoid a situation where ComputeScrollbarExistence is overloaded to do slightly different things at different stages.
So, I think ComputeScrollbarExistence is _already_ overloaded in that way. It even takes a parameter to tell it which stage it's in.
```
enum ComputeScrollbarExistenceOption {
kDependsOnOverflow,
kOverflowIndependent
};
void ComputeScrollbarExistence(
bool& needs_horizontal_scrollbar,
bool& needs_vertical_scrollbar,
ComputeScrollbarExistenceOption = kDependsOnOverflow) const;
```
... but it's a pretty clean overload, and I'll do my best to keep it clean.
Maybe we should we introduce a new struct like "StyleBasedScrollbarData", which can be computed at style recalc time and tells us if we have vw-impacting scrollbars, along with their thickness?
- Anders Hartvoll Ruud
- Steve Kobes
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. |