| Summary: | Rename overrideContentLogicalXXX() to overrideContentAndScrollbarLogicalXXX() | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||
| Component: | New Bugs | Assignee: | Sergio Villar Senin <svillar> | ||||
| Status: | RESOLVED WONTFIX | ||||||
| Severity: | Normal | CC: | changseok, esprehn+autocc, ews-watchlist, glenn, jfernandez, kondapallykalyan, pdr, rego, sam, simon.fraser | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Sergio Villar Senin
2020-06-29 10:48:02 PDT
Created attachment 403086 [details]
Patch
There was a rename for these methods in Blink some time ago, but the solution there was different (instead of changing the name, the method was changed so it actually returned the content size and not the content + scrollbar). Maybe that can be an useful thing to do in WebKit too: https://chromium-review.googlesource.com/c/chromium/src/+/1024171/ A method that returns content size + scrollbar, but doesn't include the border size doesn't look like a very useful thing to me, and quite confusing if you play with borders, scrollbars and so on. (In reply to Manuel Rego Casasnovas from comment #2) > There was a rename for these methods in Blink some time ago, but the > solution there was different (instead of changing the name, the method was > changed so it actually returned the content size and not the content + > scrollbar). Maybe that can be an useful thing to do in WebKit too: > https://chromium-review.googlesource.com/c/chromium/src/+/1024171/ Nice to know. > A method that returns content size + scrollbar, but doesn't include the > border size doesn't look like a very useful thing to me, and quite confusing > if you play with borders, scrollbars and so on. Well maybe doesn't look useful but it's what it has been doing for years :). The point of this patch was not fixing that, but making it explicit so that we don't commit mistakes while the methods are there. Comment on attachment 403086 [details]
Patch
The scrollbar is outside the padding box (see paddingBoxRectIncludingScrollbar), so I'd like a clearer vision of how this relates to contentLogicalWidth/paddingBoxRectIncludingScrollbar.
(In reply to Simon Fraser (smfr) from comment #4) > Comment on attachment 403086 [details] > Patch > > The scrollbar is outside the padding box (see > paddingBoxRectIncludingScrollbar), so I'd like a clearer vision of how this > relates to contentLogicalWidth/paddingBoxRectIncludingScrollbar. Right the situation is pretty convoluted right now as the code is breaking the css box model in the sense that it's storing the content+scrollbar bypassing the padding. I think it'd be better to follow Manuel's advice and revamp the whole override thing. Comment on attachment 403086 [details]
Patch
Removing the r?. Handled in another bug.
|