Summary: | [watchOS] Make a few additional adjustments to support system minimum layout margins | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||
Component: | Platform | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bdakin, hi, megan_gardner, thorton, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Wenson Hsieh
2021-07-11 10:37:54 PDT
Created attachment 433291 [details]
For EWS
Comment on attachment 433291 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=433291&action=review > Source/WebKit/UIProcess/ios/WKScrollView.mm:464 > + super.contentScrollInset = UIEdgeInsetsZero; Is this bit going to cause any trouble on iOS? Comment on attachment 433291 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=433291&action=review Thanks for the review! >> Source/WebKit/UIProcess/ios/WKScrollView.mm:464 >> + super.contentScrollInset = UIEdgeInsetsZero; > > Is this bit going to cause any trouble on iOS? Oh, good catch! I think that in theory, it could cause trouble if a client were setting `-_contentScrollInset` on iOS (since the interaction between `-_contentScrollInset` and `-contentInset` is…reasonable on non-watch iOS platforms). Since the need to duck `-_contentScrollInset` when a `-contentInset` is specified is really only needed on watchOS, I'll make some of this code conditional for watchOS only. That said — as this patch currently is, this method shouldn't be reachable at all on non-watchOS platforms, since the only way it can be reached is for `_contentScrollInsetInternal` to be set, and we only call `-_setContentScrollInsetInternal:` from a watchOS-specific codepath. Created attachment 433316 [details]
Patch
Committed r279830 (239591@main): <https://commits.webkit.org/239591@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433316 [details]. (In reply to Wenson Hsieh from comment #3) > Comment on attachment 433291 [details] > For EWS > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433291&action=review > > Thanks for the review! > > >> Source/WebKit/UIProcess/ios/WKScrollView.mm:464 > >> + super.contentScrollInset = UIEdgeInsetsZero; > > > > Is this bit going to cause any trouble on iOS? > > Oh, good catch! I think that in theory, it could cause trouble if a client > were setting `-_contentScrollInset` on iOS (since the interaction between > `-_contentScrollInset` and `-contentInset` is…reasonable on non-watch iOS > platforms). Since the need to duck `-_contentScrollInset` when a > `-contentInset` is specified is really only needed on watchOS, I'll make > some of this code conditional for watchOS only. Perfect! Thank you. Best to limit the counter-hacks to places where the hacks exist :) > That said — as this patch currently is, this method shouldn't be reachable > at all on non-watchOS platforms, since the only way it can be reached is for > `_contentScrollInsetInternal` to be set, and we only call > `-_setContentScrollInsetInternal:` from a watchOS-specific codepath. Sure, makes sense. |