Bug 220633 – [macOS] Titlebar separator doesn't show when WKWebView is scrolled

• Component: WebKit API
• Status: RESOLVED FIXED

[Aditya Keerthi]

Created attachment 417646 [details]
Before

As of Big Sur, NSWindow's titlebar displays a shadow divider when an NSScrollView is adjacent to the titlebar and is scrolled.

Since WKWebView's are scrollable views, the same divider should be displayed when a WKWebView is adjacent to the titlebar and is scrolled.

[Aditya Keerthi]

rdar://problem/71094055

[Aditya Keerthi]

<rdar://problem/71094055>

[Aditya Keerthi]

Created attachment 417648 [details]
After

[Aditya Keerthi]

Created attachment 417650 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 417650 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417650&action=review

> Source/WTF/wtf/PlatformHave.h:813
> +#define HAVE_NSSCROLLVIEWSEPARATORTRACKINGADAPTER 1

I feel like this should have more underscores in it.

> Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:1004
> +- (void)layout

We also do stuff in -renewGState. Should probably align them.

> Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:1018
> +- (NSRect)scrollViewFrame {

Brace on new line.

> Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:1024
> +- (BOOL)hasScrolledContentsUnderTitlebar {

Ditto

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2221
> +    [m_view didChangeValueForKey:@"scrollViewFrame"];

Should we give paired "willChange" and "didChange"?

[Tim Horton]

Comment on attachment 417650 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417650&action=review

> Source/WTF/wtf/PlatformHave.h:813
> +#define HAVE_NSSCROLLVIEWSEPARATORTRACKINGADAPTER 1

I would suggest more underscores

> Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:1011
> +- (void)geometryInWindowDidChange

Huh, what is this, and how is it different from renewGState? I wonder if this is called less and if this covers all the cases we actually care about?

> Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:1018
> +- (NSRect)scrollViewFrame {

Curly braces onto the next line

> Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:1032
> +    return [[self window] registerScrollViewSeparatorTrackingAdapter:self];

self.window

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2321
> +            m_isRegisteredScrollViewSeparatorTrackingAdapter = NO;

it's a `bool`, so `false`, not `NO`

> Source/WebKit/UIProcess/WebPageProxy.cpp:5853
> +    pageClient().pageDidScroll(scrollPosition);

This isn't going to work for full-page overflow scroll regions (pages that don't use page scrolling) but I guess that's OK

[Aditya Keerthi]

Created attachment 417657 [details]
Patch

[Aditya Keerthi]

(In reply to Tim Horton from comment #6)
> Comment on attachment 417650 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417650&action=review
> 
> > Source/WTF/wtf/PlatformHave.h:813
> > +#define HAVE_NSSCROLLVIEWSEPARATORTRACKINGADAPTER 1
> 
> I would suggest more underscores

Done.
 
> > Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:1011
> > +- (void)geometryInWindowDidChange
> 
> Huh, what is this, and how is it different from renewGState? I wonder if
> this is called less and if this covers all the cases we actually care about?

renewGState is called by geometryInWindowDidChange, so I've removed this override. The logic is now in WebViewImpl::updateWindowAndViewFrames (which is called by WebViewImpl::renewGState).

 
> > Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:1018
> > +- (NSRect)scrollViewFrame {
> 
> Curly braces onto the next line

Done.
 
> > Source/WebKit/UIProcess/API/mac/WKWebViewMac.mm:1032
> > +    return [[self window] registerScrollViewSeparatorTrackingAdapter:self];
> 
> self.window

Done.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2321
> > +            m_isRegisteredScrollViewSeparatorTrackingAdapter = NO;
> 
> it's a `bool`, so `false`, not `NO`

Done.

[Aditya Keerthi]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 417650 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417650&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2221
> > +    [m_view didChangeValueForKey:@"scrollViewFrame"];
> 
> Should we give paired "willChange" and "didChange"?

It's not necessary for the functionality, and I'm not sure where the right place to call "willChange" would be.

[Aditya Keerthi]

Created attachment 417659 [details]
Patch

[Tim Horton]

Comment on attachment 417659 [details]
Patch

Does this work right when a page comes back from the page cache?

I guess it's OK to exclude WKView, though would also be trivial to make it work since most of the implementation is (correctly) in WebViewImpl

[Aditya Keerthi]

Created attachment 418004 [details]
Patch for landing

[Aditya Keerthi]

(In reply to Tim Horton from comment #11)
> Comment on attachment 417659 [details]
> Patch
> 
> Does this work right when a page comes back from the page cache?

It does. Tested manually and also added an additional API test for this case.
 
> I guess it's OK to exclude WKView, though would also be trivial to make it
> work since most of the implementation is (correctly) in WebViewImpl

I've updated WKView to conform to the protocol.

[EWS]

Committed r271691: <https://trac.webkit.org/changeset/271691>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418004 [details].

[Truitt Savell]

The changes in https://trac.webkit.org/changeset/271691/webkit

broke 22 API test on Big Sur
Build: https://build.webkit.org/builders/Apple-BigSur-Release-WK2-Tests/builds/1077

I am able to reproduce this easily by just running the tests.

[Aditya Keerthi]

Reverted in https://trac.webkit.org/changeset/271713/

[Aditya Keerthi]

Created attachment 428880 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 428880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428880&action=review

Does all this work if a view is patently while already scrolled?

> Source/WebKit/UIProcess/API/mac/WKView.mm:1149
> +- (BOOL)hasScrolledContentsUnderTitlebar

This naming is a little ambiguous Does it mean "contents scrolled under the title bar in the recent past" or "this view has contents which are scrolled under the title bar"? Could it instead be called -scrolledToTop?

> Source/WebKit/UIProcess/API/mac/WKView.mm:1158
> +    return [self.window registerScrollViewSeparatorTrackingAdapter:self];

Does this handle multiple web views in one window (say, side by side and adjacent to the toolbar)?

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.h:880
> +    bool m_pageIsScrolled { false };

Not sure if this means "not scrolled to top" or "has been scrolled at all".

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1825
> +    [m_view didChangeValueForKey:@"scrollViewFrame"];

Do we need paired will/did change value for key?

[Simon Fraser (smfr)]

^ is parented

[Aditya Keerthi]

Created attachment 428907 [details]
Patch

[Aditya Keerthi]

(In reply to Simon Fraser (smfr) from comment #18)
> Comment on attachment 428880 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428880&action=review
> 
> Does all this work if a view is patently while already scrolled?

It does. I've added an API test to exercise this scenario.
 
> > Source/WebKit/UIProcess/API/mac/WKView.mm:1149
> > +- (BOOL)hasScrolledContentsUnderTitlebar
> 
> This naming is a little ambiguous Does it mean "contents scrolled under the
> title bar in the recent past" or "this view has contents which are scrolled
> under the title bar"? Could it instead be called -scrolledToTop?

It means "this view has contents which are scrolled under the title bar".

Unfortunately, we don't control this name, since it's a delegate method owned by AppKit.
 
> > Source/WebKit/UIProcess/API/mac/WKView.mm:1158
> > +    return [self.window registerScrollViewSeparatorTrackingAdapter:self];
> 
> Does this handle multiple web views in one window (say, side by side and
> adjacent to the toolbar)?

Yes, in the case where there are multiple web views in one window, we don't want to show the separator at all. This logic is handled in AppKit, by `NSWindowSectionController`. The separator is only supposed to be shown if the view is scrolled, and takes up the entire window section.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.h:880
> > +    bool m_pageIsScrolled { false };
> 
> Not sure if this means "not scrolled to top" or "has been scrolled at all".

Renamed to m_pageIsScrolledToTop (with a default "true"), to avoid confusion.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1825
> > +    [m_view didChangeValueForKey:@"scrollViewFrame"];
> 
> Do we need paired will/did change value for key?

Copying from Comment 9: "It's not necessary for the functionality, and I'm not sure where the right place to call "willChange" would be".

[Darin Adler]

Comment on attachment 428907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428907&action=review

> Source/WebKit/UIProcess/API/mac/WKView.mm:73
> +@interface WKView() <NSScrollViewSeparatorTrackingAdapter>

Normally we put a space before the category name, so "WKView ()".

Does declaring it this way result in checking that we remember to implement all the methods below? If not, then maybe we should consider a named category instead so that’s checked for us.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.h:152
> +#if HAVE(NSSCROLLVIEW_SEPARATOR_TRACKING_ADAPTER)
> +- (BOOL)_web_registerScrollViewSeparatorTrackingAdapter;
> +- (void)_web_unregisterScrollViewSeparatorTrackingAdapter;
> +#endif

I don’t think these methods are needed. I think the calling code can call -[NSWindow registerScrollViewSeparatorTrackingAdapter:] and -[NSWindow unregisterScrollViewSeparatorTrackingAdapter:]. No need to add this specially-named method to have the object make the method call.

If we are worried about robustness and not registering a view that might not implement the protocol, we can check conformsToProtocol: explicitly.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1825
> +    [m_view didChangeValueForKey:@"scrollViewFrame"];

This seems to always claim it’s changed even if no change occurred. Is it OK to do it that way? Why not send the notification only if something changes, instead?

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2329
> +        if ([m_view respondsToSelector:@selector(_web_unregisterScrollViewSeparatorTrackingAdapter)]) {

I think we don’t need this check. If the view doesn’t respond to this selector, then m_isRegisteredScrollViewSeparatorTrackingAdapter won’t be true.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2417
> +    updateTitlebarAdjacencyState();

Why isn’t didChangeValueForKey:@"scrollViewFrame" needed here?

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2426
> +    updateTitlebarAdjacencyState();

Ditto.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2439
> +    if ((scrollPosition.y() > 0 && m_pageIsScrolledToTop) || (scrollPosition.y() <= 0 && !m_pageIsScrolledToTop)) {

This should use != rather than the two boolean checks. Something like this (although I have have gotten the logic backwards.

    if ((scrollPosition.y() <= 0) != m_pageIsScrolledToTop) {

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2461
> +    BOOL wantsScrollViewSeparatorTrackingAdapterRegistration = false;

In C++ files we should probably use bool instead of BOOL.

I would name this "need" instead of "wants" or "should register". In fact in this short function I think I would literally name this "shouldRegister". Context tells the reader what we should register: "this view as a scroll view separator tracking adapter".

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2463
> +    NSWindow *window = [m_view window];

We don’t need this in a local variable. It’s only used once.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2465
> +    NSRect windowContentLayoutRectInSelf = [m_view convertRect:[window contentLayoutRect] fromView:nil];

We don’t need this whole rectangle in a local variable. Instead we should just put the NSMinY in the variable. Could give it an easier to understand name.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2466
> +    BOOL topOfWindowContentLayoutRectAdjacent = (NSMinY([m_view bounds]) <= NSMinY(windowContentLayoutRectInSelf));

Not sure we need the outer parentheses here.

The idea of using the bounds and converting the content layout rectangle to self doesn’t seem logical. Seems more logical to use frame and convert the contentLayoutRect to the superview coordinate system. The frame is how we think of the rectangle in the context of abutting other views. The bounds is more how we think of the rectangle within our own drawing logic. On the other hand, this logic is likely correct and changing it might just mess the logic up.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2473
> +        if ([m_view respondsToSelector:@selector(_web_registerScrollViewSeparatorTrackingAdapter)])
> +            m_isRegisteredScrollViewSeparatorTrackingAdapter = [m_view _web_registerScrollViewSeparatorTrackingAdapter];

I would suggest using && instead of an if statement here.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2475
> +        if ([m_view respondsToSelector:@selector(_web_unregisterScrollViewSeparatorTrackingAdapter)]) {

Since we already checked m_isRegisteredScrollViewSeparatorTrackingAdapter, this respondsToSelector: check isn’t needed or helpful. If the view responds to _web_registerScrollViewSeparatorTrackingAdapter but not _web_unregisterScrollViewSeparatorTrackingAdapter, then we have something truly strange going on.

[Aditya Keerthi]

Created attachment 428958 [details]
Patch for landing

[Aditya Keerthi]

(In reply to Darin Adler from comment #22)
> Comment on attachment 428907 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428907&action=review
> 
> > Source/WebKit/UIProcess/API/mac/WKView.mm:73
> > +@interface WKView() <NSScrollViewSeparatorTrackingAdapter>
> 
> Normally we put a space before the category name, so "WKView ()".

Done.
 
> Does declaring it this way result in checking that we remember to implement
> all the methods below? If not, then maybe we should consider a named
> category instead so that’s checked for us.

It does, the compiler complains if I leave one of the delegate methods unimplemented.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.h:152
> > +#if HAVE(NSSCROLLVIEW_SEPARATOR_TRACKING_ADAPTER)
> > +- (BOOL)_web_registerScrollViewSeparatorTrackingAdapter;
> > +- (void)_web_unregisterScrollViewSeparatorTrackingAdapter;
> > +#endif
> 
> I don’t think these methods are needed. I think the calling code can call
> -[NSWindow registerScrollViewSeparatorTrackingAdapter:] and -[NSWindow
> unregisterScrollViewSeparatorTrackingAdapter:]. No need to add this
> specially-named method to have the object make the method call.
> 
> If we are worried about robustness and not registering a view that might not
> implement the protocol, we can check conformsToProtocol: explicitly.

I've removed these methods.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1825
> > +    [m_view didChangeValueForKey:@"scrollViewFrame"];
> 
> This seems to always claim it’s changed even if no change occurred. Is it OK
> to do it that way? Why not send the notification only if something changes,
> instead?

It's technically okay, because it has no impact on correctness. However, always sending the notification does result in AppKit running more logic than necessary

I've added a variable that keeps track of the previous value, so that we don't send more notifications than necessary.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2329
> > +        if ([m_view respondsToSelector:@selector(_web_unregisterScrollViewSeparatorTrackingAdapter)]) {
> 
> I think we don’t need this check. If the view doesn’t respond to this
> selector, then m_isRegisteredScrollViewSeparatorTrackingAdapter won’t be
> true.

Removed.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2417
> > +    updateTitlebarAdjacencyState();
> 
> Why isn’t didChangeValueForKey:@"scrollViewFrame" needed here?

This is in viewDidHide(), the frame has not changed. This also matches the logic in NSScrollView.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2426
> > +    updateTitlebarAdjacencyState();
> 
> Ditto.

This is in viewDidUnhide(), the frame has not changed. This also matches the logic in NSScrollView.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2439
> > +    if ((scrollPosition.y() > 0 && m_pageIsScrolledToTop) || (scrollPosition.y() <= 0 && !m_pageIsScrolledToTop)) {
> 
> This should use != rather than the two boolean checks. Something like this
> (although I have have gotten the logic backwards.
> 
>     if ((scrollPosition.y() <= 0) != m_pageIsScrolledToTop) {

This is a lot cleaner! And your logic is correct :)
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2461
> > +    BOOL wantsScrollViewSeparatorTrackingAdapterRegistration = false;
> 
> In C++ files we should probably use bool instead of BOOL.

Done.
 
> I would name this "need" instead of "wants" or "should register". In fact in
> this short function I think I would literally name this "shouldRegister".
> Context tells the reader what we should register: "this view as a scroll
> view separator tracking adapter".

Renamed to "shouldRegister".
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2463
> > +    NSWindow *window = [m_view window];
> 
> We don’t need this in a local variable. It’s only used once.

I kept the local variable, since it's used more than once now that we're directly calling [NSWindow registerScrollViewSeparatorTrackingAdapter:] here.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2465
> > +    NSRect windowContentLayoutRectInSelf = [m_view convertRect:[window contentLayoutRect] fromView:nil];
> 
> We don’t need this whole rectangle in a local variable. Instead we should
> just put the NSMinY in the variable. Could give it an easier to understand
> name.

Done.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2466
> > +    BOOL topOfWindowContentLayoutRectAdjacent = (NSMinY([m_view bounds]) <= NSMinY(windowContentLayoutRectInSelf));
> 
> Not sure we need the outer parentheses here.

Removed.
 
> The idea of using the bounds and converting the content layout rectangle to
> self doesn’t seem logical. Seems more logical to use frame and convert the
> contentLayoutRect to the superview coordinate system. The frame is how we
> think of the rectangle in the context of abutting other views. The bounds is
> more how we think of the rectangle within our own drawing logic. On the
> other hand, this logic is likely correct and changing it might just mess the
> logic up.

I kept this logic as-is, since it's directly copied from NSScrollView's adoption of NSScrollViewSeparatorTrackingAdapter.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2473
> > +        if ([m_view respondsToSelector:@selector(_web_registerScrollViewSeparatorTrackingAdapter)])
> > +            m_isRegisteredScrollViewSeparatorTrackingAdapter = [m_view _web_registerScrollViewSeparatorTrackingAdapter];
> 
> I would suggest using && instead of an if statement here.

Done.
 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2475
> > +        if ([m_view respondsToSelector:@selector(_web_unregisterScrollViewSeparatorTrackingAdapter)]) {
> 
> Since we already checked m_isRegisteredScrollViewSeparatorTrackingAdapter,
> this respondsToSelector: check isn’t needed or helpful. If the view responds
> to _web_registerScrollViewSeparatorTrackingAdapter but not
> _web_unregisterScrollViewSeparatorTrackingAdapter, then we have something
> truly strange going on.

Removed the check.

[Darin Adler]

Comment on attachment 428958 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=428958&action=review

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2338
> +        [currentWindow unregisterScrollViewSeparatorTrackingAdapter:(NSObject<NSScrollViewSeparatorTrackingAdapter> *)m_view.getAutoreleased()];

This should not require getAutoreleased(). We need autorelease for return values, not for function arguments. Just plain get() should be fine, and more efficient.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2471
> +    BOOL visible = ![m_view isHiddenOrHasHiddenAncestor];

bool instead of BOOL

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2473
> +    BOOL topOfWindowContentLayoutRectAdjacent = NSMinY([m_view bounds]) <= windowContentLayoutRectInSelfMinY;

bool instead of BOOL

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2476
> +    if (topOfWindowContentLayoutRectAdjacent && visible && [[m_view effectiveAppearance] _usesMetricsAppearance])
> +        shouldRegister = true;

Better to use assignment and not an if:

    bool shouldRegister = topOfWindowContentLayoutRectAdjacent && visible && m_view.effectiveAppearance._usesMetricsAppearance;

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2479
> +        m_isRegisteredScrollViewSeparatorTrackingAdapter = [window registerScrollViewSeparatorTrackingAdapter:(NSObject<NSScrollViewSeparatorTrackingAdapter> *)m_view.getAutoreleased()];

Ditto.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2481
> +        [window unregisterScrollViewSeparatorTrackingAdapter:(NSObject<NSScrollViewSeparatorTrackingAdapter> *)m_view.getAutoreleased()];

Ditto.

[Darin Adler]

Comment on attachment 428958 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=428958&action=review

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2479
>> +        m_isRegisteredScrollViewSeparatorTrackingAdapter = [window registerScrollViewSeparatorTrackingAdapter:(NSObject<NSScrollViewSeparatorTrackingAdapter> *)m_view.getAutoreleased()];
> 
> Ditto.

I meant "get not getAutoreleased".

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2481
>> +        [window unregisterScrollViewSeparatorTrackingAdapter:(NSObject<NSScrollViewSeparatorTrackingAdapter> *)m_view.getAutoreleased()];
> 
> Ditto.

I meant "get not getAutoreleased".

[Aditya Keerthi]

Created attachment 428971 [details]
Patch for landing

[EWS]

Committed r277688 (237884@main): <https://commits.webkit.org/237884@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428971 [details].