RESOLVED WONTFIX213743
Rename overrideContentLogicalXXX() to overrideContentAndScrollbarLogicalXXX()
https://bugs.webkit.org/show_bug.cgi?id=213743
Summary Rename overrideContentLogicalXXX() to overrideContentAndScrollbarLogicalXXX()
Sergio Villar Senin
Reported 2020-06-29 10:48:02 PDT
Rename overrideContentLogicalXXX() to overrideContentAndScrollbarLogicalXXX()
Attachments
Patch (45.10 KB, patch)
2020-06-29 11:03 PDT, Sergio Villar Senin
no flags
Sergio Villar Senin
Comment 1 2020-06-29 11:03:31 PDT
Manuel Rego Casasnovas
Comment 2 2020-06-30 01:06:53 PDT
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.
Sergio Villar Senin
Comment 3 2020-06-30 01:16:29 PDT
(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.
Simon Fraser (smfr)
Comment 4 2020-06-30 10:57:38 PDT
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.
Sergio Villar Senin
Comment 5 2020-07-06 01:29:42 PDT
(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.
Sergio Villar Senin
Comment 6 2022-01-17 01:13:01 PST
Comment on attachment 403086 [details] Patch Removing the r?. Handled in another bug.
Note You need to log in before you can comment on or make changes to this bug.