WebKit Bugzilla
New
Browse
Search+
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
213743
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
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1
2020-06-29 11:03:31 PDT
Created
attachment 403086
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug