Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Find in Note: Dark gray outline (shadow) appears behind gray/yellow highlights when matched text found in HTML note. #25600

Conversation

megangardner
Copy link
Contributor

@megangardner megangardner commented Mar 7, 2024

89bc7dd

Find in Note: Dark gray outline (shadow) appears behind gray/yellow highlights when matched text found in HTML note.
https://bugs.webkit.org/show_bug.cgi?id=270666
rdar://122843511

Reviewed by Aditya Keerthi.

In notes, the WKContentView is transparent, so our original solution of putting an additional
grey layer behind the content view that filled up the empty parts of the scroll view would show
through and make the find ui have a incorrect grey cast. So instead, we make four views that surround
the WKContentView to fill in any part of the scrollView that isn't covered by the contentView.
These are arranged around the content view like so:

----- -----------
|    |          |
|    |----------|
|    |     |    |
|    |     |    |
----- ------    |
|          |    |
|__________|____|

Each view is expanded to reach the edges of the scroll view every time the view is scrolled or the bounds change.
This means that no matter where the content view is scrolled to, there will be a view that gives the correct
grey cast to the scroll view.

* Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h:
* Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:
(-[WKWebView scrollViewDidScroll:]):
(-[WKWebView _frameOrBoundsMayHaveChanged]):
(-[WKWebView _updateFindOverlayForOverflowScrollPositions]):
(-[WKWebView _showFindOverlay]):
(-[WKWebView _hideFindOverlay]):
(-[WKWebView _didAddLayerForFindOverlay:]):
(-[WKWebView _updateFindOverlayPosition]): Deleted.

Canonical link: https://commits.webkit.org/275873@main

e2c2fca

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ›  wpe-skia
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@megangardner megangardner self-assigned this Mar 7, 2024
@megangardner megangardner added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Mar 7, 2024
@@ -201,6 +201,15 @@ struct PerWebProcessState {

#endif // PLATFORM(IOS_FAMILY)

#if HAVE(UIFINDINTERACTION)
enum FindOverlayView {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be an enum class : uint8_t ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe it should just be a struct instead of an enum + a loosely-ordered vector? like

struct FindOverlayViews {
 RetainPtr<UIView> left;
// etc
}

@@ -201,6 +201,15 @@ struct PerWebProcessState {

#endif // PLATFORM(IOS_FAMILY)

#if HAVE(UIFINDINTERACTION)
enum FindOverlayView {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
enum FindOverlayView {
enum class FindOverlayViewPosition : uint8_t {

Probably best to also move this to the .mm file, since it isn't needed outside of it.

@@ -247,7 +256,7 @@ struct PerWebProcessState {

BOOL _findInteractionEnabled;
#if HAVE(UIFINDINTERACTION)
RetainPtr<UIView> _findOverlay;
Vector<RetainPtr<UIView>> _findOverlaysForOverflowScroll; // Top, right, bottom, left
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"OverflowScroll" is bit of an overloaded term here, since this is not really about overflow: scroll.

_findOverlaysOutsideContentView might be more accurate.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 7, 2024
Copy link
Member

@aprotyas aprotyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix LGTM actually -- I'm suggesting an alternative approach to manage the overlays

@@ -201,6 +201,15 @@ struct PerWebProcessState {

#endif // PLATFORM(IOS_FAMILY)

#if HAVE(UIFINDINTERACTION)
enum FindOverlayView {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this an enum class please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see why you did it like this. My other comment speaks in a different direction if you want to consider that.

@@ -247,7 +256,7 @@ struct PerWebProcessState {

BOOL _findInteractionEnabled;
#if HAVE(UIFINDINTERACTION)
RetainPtr<UIView> _findOverlay;
Vector<RetainPtr<UIView>> _findOverlaysForOverflowScroll; // Top, right, bottom, left
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overlay ordering enforcement is a little fragile... particularly since it's documented through a comment. What do you think about introducing a structure that captures what is going on?

struct OverflowScrollOverlays {
    RetainPtr<UIView> top;
    RetainPtr<UIView> right;
    RetainPtr<UIView> bottom;
    RetainPtr<UIView> left;
};

OverflowScrollOverlays _findOverlaysForOverflowScroll;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That way you can also get rid of enum FindOverlayView?

Comment on lines 3360 to 3363
auto topView = _findOverlaysForOverflowScroll[FindOverlayView::Top].get();
auto rightView = _findOverlaysForOverflowScroll[FindOverlayView::Right].get();
auto bottomView = _findOverlaysForOverflowScroll[FindOverlayView::Bottom].get();
auto leftView = _findOverlaysForOverflowScroll[FindOverlayView::Left].get();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our smart pointer guidelines suggest these should remain as smart pointers on the stack, so I think we prefer:

Suggested change
auto topView = _findOverlaysForOverflowScroll[FindOverlayView::Top].get();
auto rightView = _findOverlaysForOverflowScroll[FindOverlayView::Right].get();
auto bottomView = _findOverlaysForOverflowScroll[FindOverlayView::Bottom].get();
auto leftView = _findOverlaysForOverflowScroll[FindOverlayView::Left].get();
RetainPtr topView = _findOverlaysForOverflowScroll[FindOverlayView::Top];
RetainPtr rightView = _findOverlaysForOverflowScroll[FindOverlayView::Right];
RetainPtr bottomView = _findOverlaysForOverflowScroll[FindOverlayView::Bottom];
RetainPtr leftView = _findOverlaysForOverflowScroll[FindOverlayView::Left];

UIColor *overlayColor = [UIColor colorWithRed:(26. / 255) green:(26. / 255) blue:(26. / 255) alpha:(64. / 255)];
[_findOverlay setBackgroundColor:overlayColor];
UIColor *overlayColor = [UIColor colorWithRed:(26. / 255) green:(26. / 255) blue:(26. / 255) alpha:(64. / 255)];
if (!_findOverlaysForOverflowScroll.size()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: _findOverlaysForOverflowScroll.isEmpty() is a little easier to read

_findOverlay = adoptNS([[UIView alloc] init]);
UIColor *overlayColor = [UIColor colorWithRed:(26. / 255) green:(26. / 255) blue:(26. / 255) alpha:(64. / 255)];
[_findOverlay setBackgroundColor:overlayColor];
UIColor *overlayColor = [UIColor colorWithRed:(26. / 255) green:(26. / 255) blue:(26. / 255) alpha:(64. / 255)];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be moved inside the if.


if (CALayer *contentViewFindOverlayLayer = [self _layerForFindOverlay]) {
[[_findOverlay layer] removeAllAnimations];
[contentViewFindOverlayLayer removeAllAnimations];
for (RetainPtr overlay : _findOverlaysForOverflowScroll) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the repetition of this loop, we could have a method like _updateFindOverlaysOutsideContentView: that takes a block, and calls the block on each view.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or just make it an initializer list like for (RetainPtr overlay : { _findOverlaysForOverflowScroll.left, _findOverlaysForOverflowScroll.right, etc {) ?


[strongSelf _removeLayerForFindOverlay];
}];

[contentViewFindOverlayLayer addAnimation:animation forKey:@"findOverlayFadeOut"];
[findOverlayLayer addAnimation:animation forKey:@"findOverlayFadeOut"];
for (RetainPtr overlay : _findOverlaysForOverflowScroll)
[overlay addAnimation:animation forKey:@"findOverlayFadeOut"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[overlay addAnimation:animation forKey:@"findOverlayFadeOut"];
[[overlay layer] addAnimation:animation forKey:@"findOverlayFadeOut"];

@megangardner megangardner removed the merging-blocked Applied to prevent a change from being merged label Mar 8, 2024
@megangardner megangardner force-pushed the eng/Find-in-Note-Dark-gray-outline-shadow-appears-behind-grayyellow-highlights-when-matched-text-found-in-HTML-note- branch from d56bafe to 9ff9300 Compare March 8, 2024 03:59
@@ -247,7 +247,13 @@ struct PerWebProcessState {

BOOL _findInteractionEnabled;
#if HAVE(UIFINDINTERACTION)
RetainPtr<UIView> _findOverlay;
struct findOverlays {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should be FindOverlays


[CATransaction commit];

contentViewFindOverlayLayer.opacity = 0;
findOverlayLayer.opacity = 0;
[_findOverlaysOutsideContentView.value().top layer].opacity = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anywhere you use value() should just be ->.

But please also try to do this #25600 (comment), so that only needs to happen in one place.

@megangardner megangardner force-pushed the eng/Find-in-Note-Dark-gray-outline-shadow-appears-behind-grayyellow-highlights-when-matched-text-found-in-HTML-note- branch from 9ff9300 to 9d068dd Compare March 8, 2024 04:42
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 8, 2024
@megangardner megangardner removed the merging-blocked Applied to prevent a change from being merged label Mar 8, 2024
@megangardner megangardner force-pushed the eng/Find-in-Note-Dark-gray-outline-shadow-appears-behind-grayyellow-highlights-when-matched-text-found-in-HTML-note- branch from 9d068dd to 58f0910 Compare March 8, 2024 04:54
[self _addLayerForFindOverlay];
}
}

- (void)_hideFindOverlay
{
CALayer *contentViewFindOverlayLayer = [self _layerForFindOverlay];
CALayer *findOverlayLayer = [_findOverlay layer];
CALayer *findOverlayLayer = [_findOverlaysOutsideContentView.value().top layer];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What guarantees _findOverlaysOutsideContentView is not std::nullopt here? I think you need to update the check of line 3430 to check for that, and then move this line after the early return – otherwise this is a change in logic.

[self _addLayerForFindOverlay];
}
}

- (void)_hideFindOverlay
{
CALayer *contentViewFindOverlayLayer = [self _layerForFindOverlay];
CALayer *findOverlayLayer = [_findOverlay layer];
CALayer *findOverlayLayer = [_findOverlaysOutsideContentView.value().top layer];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also -> rather than value().

@@ -3559,20 +3608,33 @@ - (_UIDataOwner)_effectiveDataOwner:(_UIDataOwner)clientSuppliedDataOwner
return _UIDataOwnerUndefined;
}

#if HAVE(UIFINDINTERACTION)
- (void)_updateFindOverlaysOutsideContentView:(void(^)(RetainPtr<UIView>))updateFindOverlays
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- (void)_updateFindOverlaysOutsideContentView:(void(^)(RetainPtr<UIView>))updateFindOverlays
- (void)_updateFindOverlaysOutsideContentView:(void(^)(UIView *))updateFindOverlays

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be UIView * rather than RetainPtr to avoid ref-churn.

Comment on lines 3614 to 3617
updateFindOverlays(_findOverlaysOutsideContentView->top);
updateFindOverlays(_findOverlaysOutsideContentView->bottom);
updateFindOverlays(_findOverlaysOutsideContentView->left);
updateFindOverlays(_findOverlaysOutsideContentView->right);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
updateFindOverlays(_findOverlaysOutsideContentView->top);
updateFindOverlays(_findOverlaysOutsideContentView->bottom);
updateFindOverlays(_findOverlaysOutsideContentView->left);
updateFindOverlays(_findOverlaysOutsideContentView->right);
updateFindOverlay(_findOverlaysOutsideContentView->top);
updateFindOverlay(_findOverlaysOutsideContentView->bottom);
updateFindOverlay(_findOverlaysOutsideContentView->left);
updateFindOverlay(_findOverlaysOutsideContentView->right);

[contentViewFindOverlayLayer setOpacity:1];
} else {
[_findOverlay setAlpha:0];
if (_findOverlaysOutsideContentView) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this check is necessary? The variable is always initialized by this method.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 8, 2024
@megangardner megangardner removed the merging-blocked Applied to prevent a change from being merged label Mar 8, 2024
@megangardner megangardner force-pushed the eng/Find-in-Note-Dark-gray-outline-shadow-appears-behind-grayyellow-highlights-when-matched-text-found-in-HTML-note- branch from 58f0910 to 2ccc05f Compare March 8, 2024 17:44
@megangardner megangardner force-pushed the eng/Find-in-Note-Dark-gray-outline-shadow-appears-behind-grayyellow-highlights-when-matched-text-found-in-HTML-note- branch from 2ccc05f to b299aa4 Compare March 8, 2024 18:21
@@ -3404,22 +3445,28 @@ - (void)_hideFindOverlay
if (!strongSelf)
return;

if ([strongSelf->_findOverlay alpha])
return;
[strongSelf _removeLayerForFindOverlay];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be after the early return, as it was before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is in the right place, I added the other early return to check of the new overlays are in place before calling remove from them. It's possible, but unlikely, that we have an overlay layer without the overlay views, and I don't want to not remove the layer just because the views aren't there. (Or I could be misunderstanding your concern.)

Copy link
Member

@pxlcoder pxlcoder Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible to have the overlay views outside the content view without the PageOverlay layer, but not vice-versa. This is because _showFindOverlay always creates the overlay views outside the content view, before creating the PageOverlay.

I think the existing structure of the code is necessary in order to guard against scenarios where the find overlay is brought up again as it's animating out – which is possible due to the async nature of the code.

So the way this code needs to be written to preserve the existing structure is:

bool hasOverlaysOutsideContentView = strongSelf->_findOverlaysOutsideContentView.has_value();
if (hasOverlaysOutsideContentView && [strongSelf->_findOverlaysOutsideContentView->top alpha]])
    return;

[strongSelf _removeLayerForFindOverlay];
 
if (hasOverlaysOutsideContentView) {
    [strongSelf _updateFindOverlaysOutsideContentView:^(UIView *view) {
        [view removeFromSuperview];
    }];
    strongSelf->_findOverlaysOutsideContentView.reset();
}

@megangardner megangardner force-pushed the eng/Find-in-Note-Dark-gray-outline-shadow-appears-behind-grayyellow-highlights-when-matched-text-found-in-HTML-note- branch from b299aa4 to 7c64366 Compare March 8, 2024 19:12
@megangardner megangardner added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Mar 8, 2024
@megangardner megangardner force-pushed the eng/Find-in-Note-Dark-gray-outline-shadow-appears-behind-grayyellow-highlights-when-matched-text-found-in-HTML-note- branch from 7c64366 to e2c2fca Compare March 9, 2024 07:03
@webkit-ews-buildbot webkit-ews-buildbot added merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Mar 9, 2024
@webkit-ews-buildbot
Copy link
Collaborator

Safe-Merge-Queue: Build #14419.

…ighlights when matched text found in HTML note.

https://bugs.webkit.org/show_bug.cgi?id=270666
rdar://122843511

Reviewed by Aditya Keerthi.

In notes, the WKContentView is transparent, so our original solution of putting an additional
grey layer behind the content view that filled up the empty parts of the scroll view would show
through and make the find ui have a incorrect grey cast. So instead, we make four views that surround
the WKContentView to fill in any part of the scrollView that isn't covered by the contentView.
These are arranged around the content view like so:

----- -----------
|    |          |
|    |----------|
|    |     |    |
|    |     |    |
----- ------    |
|          |    |
|__________|____|

Each view is expanded to reach the edges of the scroll view every time the view is scrolled or the bounds change.
This means that no matter where the content view is scrolled to, there will be a view that gives the correct
grey cast to the scroll view.

* Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h:
* Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:
(-[WKWebView scrollViewDidScroll:]):
(-[WKWebView _frameOrBoundsMayHaveChanged]):
(-[WKWebView _updateFindOverlayForOverflowScrollPositions]):
(-[WKWebView _showFindOverlay]):
(-[WKWebView _hideFindOverlay]):
(-[WKWebView _didAddLayerForFindOverlay:]):
(-[WKWebView _updateFindOverlayPosition]): Deleted.

Canonical link: https://commits.webkit.org/275873@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Find-in-Note-Dark-gray-outline-shadow-appears-behind-grayyellow-highlights-when-matched-text-found-in-HTML-note- branch from e2c2fca to 89bc7dd Compare March 9, 2024 09:46
@webkit-commit-queue
Copy link
Collaborator

Committed 275873@main (89bc7dd): https://commits.webkit.org/275873@main

Reviewed commits have been landed. Closing PR #25600 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 89bc7dd into WebKit:main Mar 9, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
7 participants