Bug 231161 - REGRESSION (iOS 15): Safari shows zoom callout even if -webkit-user-select is none
Summary: REGRESSION (iOS 15): Safari shows zoom callout even if -webkit-user-select is...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari 15
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-04 02:39 PDT by timocov
Modified: 2022-04-14 15:12 PDT (History)
12 users (show)

See Also:


Attachments
debug page (845 bytes, text/html)
2021-10-04 02:39 PDT, timocov
no flags Details
Patch (14.57 KB, patch)
2021-10-23 16:30 PDT, Wenson Hsieh
darin: review+
Details | Formatted Diff | Diff
For landing (14.71 KB, patch)
2021-10-24 12:09 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description timocov 2021-10-04 02:39:32 PDT
Created attachment 440044 [details]
debug page

1. Open the attached page with Safari v15+ on mobile device
2. Long press anywhere at the page

=> Even the page is marked as non-selectable Safari still shows a callout.

It is simplest example, which worked in v14. In our example we want to prevent user selection for canvas elements since there is nothing to select, but still no luck.
Comment 1 Radar WebKit Bug Importer 2021-10-04 17:25:06 PDT
<rdar://problem/83863266>
Comment 2 Wenson Hsieh 2021-10-04 17:32:08 PDT
The (perhaps unfortunately named) CSS property `-webkit-touch-callout` controls whether or not we should present a context menu when long pressing over a particular element.

It sounded like you might want `-webkit-user-select: none;` instead, but while this does prevent text selection, it doesn't prevent the loupe gesture magnifier from popping up :/

One thing that does seem to work is adding an active "touchstart" event listener and preventing default.
Comment 3 timocov 2021-10-06 08:30:12 PDT
Unfortunately, preventing touchstart is not an option for our application since it might prevent the page scroll or some other gestures.

We have `-webkit-user-select: none;` as well, but it doesn’t work either.

Is there any way to get it fixed in the following ios updates? This really frustrates our users, but we cannot do anything with that since it looks like the browser’s bug.
Comment 4 Wenson Hsieh 2021-10-20 08:42:40 PDT
(In reply to timocov from comment #3)
> Unfortunately, preventing touchstart is not an option for our application
> since it might prevent the page scroll or some other gestures.
> 
> We have `-webkit-user-select: none;` as well, but it doesn’t work either.
> 
> Is there any way to get it fixed in the following ios updates? This really
> frustrates our users, but we cannot do anything with that since it looks
> like the browser’s bug.

It looks like the new magnifier UI that appears during a loupe (i.e. long-press) gesture is new behavior in iOS 15. I'll see if we can make it so that `-webkit-user-select: none;` suppresses this UI (in addition to text selection in general).
Comment 5 Joseph Pearson 2021-10-20 12:47:11 PDT
Thanks Wenson, we'd also appreciate this change, mostly for accessibility reasons. I've seen discussion of it in a few other places, including here: https://github.com/Leaflet/Leaflet/issues/7678

Note that when the loupe appears in iOS 15, a haptic thud occurs around the same time. However, this haptic has been happening for a few versions of iOS (I think it started with iOS 14.0). Prior to iOS 15, it had no visible effect if user-select=none. So I'm not sure if it is related to the loupe, but ideally the haptic would be suppressed by -webkit-user-select: none as well. If it isn't related, let me know and I'll create a separate ticket.
Comment 6 Wenson Hsieh 2021-10-23 16:30:27 PDT
Created attachment 442284 [details]
Patch
Comment 7 Darin Adler 2021-10-23 22:14:14 PDT
Comment on attachment 442284 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2898
> +            if (isAssistableElement(*downcast<Element>(hitNode)))

Not too fond of checking if not text but then casting to Element. Why not make the check and the downcast match? Also could use *node.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2912
> +        return;

Do we want to set isSelected to false here? Or is it valuable to leave the other fields with old values? Or are these always set to the default values in practice? Can’t tell from this function alone, but maybe due to how it’s used.
Comment 8 Wenson Hsieh 2021-10-24 11:46:01 PDT
Comment on attachment 442284 [details]
Patch

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

Thanks for the review!

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2898
>> +            if (isAssistableElement(*downcast<Element>(hitNode)))
> 
> Not too fond of checking if not text but then casting to Element. Why not make the check and the downcast match? Also could use *node.

Good catch — this should probably be an `is<Element>()` check instead (I moved this logic from its current place in the method, below).

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2912
>> +        return;
> 
> Do we want to set isSelected to false here? Or is it valuable to leave the other fields with old values? Or are these always set to the default values in practice? Can’t tell from this function alone, but maybe due to how it’s used.

Ah, so we never populated the `isSelected` member before in this case, which means that it's currently just the default value of `false` in the case of links and images. In practice, we don't currently consult that bit in the case where `info.isLink || info.isImage`, but I agree that it would be better to just set that value to prevent confusion down the line. I'll move the `info.isSelected = result.isSelected();` to before this early return.

(I suppose the same applies to some of the other members in this struct, such as `idAttribute` below — seems like it might be worth doing an audit at some point and make sure these are populated in all codepaths, as long as they're relatively cheap to compute).
Comment 9 Wenson Hsieh 2021-10-24 12:09:57 PDT
Created attachment 442316 [details]
For landing
Comment 10 EWS 2021-10-24 13:39:43 PDT
Committed r284766 (243475@main): <https://commits.webkit.org/243475@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442316 [details].
Comment 11 Darin Adler 2021-10-24 14:21:02 PDT
Comment on attachment 442284 [details]
Patch

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

>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2912
>>> +        return;
>> 
>> Do we want to set isSelected to false here? Or is it valuable to leave the other fields with old values? Or are these always set to the default values in practice? Can’t tell from this function alone, but maybe due to how it’s used.
> 
> Ah, so we never populated the `isSelected` member before in this case, which means that it's currently just the default value of `false` in the case of links and images. In practice, we don't currently consult that bit in the case where `info.isLink || info.isImage`, but I agree that it would be better to just set that value to prevent confusion down the line. I'll move the `info.isSelected = result.isSelected();` to before this early return.
> 
> (I suppose the same applies to some of the other members in this struct, such as `idAttribute` below — seems like it might be worth doing an audit at some point and make sure these are populated in all codepaths, as long as they're relatively cheap to compute).

Yes, that’s sort of what I had in mind.

One way to do this is to have a structure that initializes all its data members in its definition or a constructor, then have the structure be a return value so we know we are filling in a default-initialized local variable and don’t have to worry about data members we don’t set. But in the case of this function, I believe this is actually an in/out case where the structure is partially filled in by the caller and is not just a pure return value.

Maybe there’s no practical problem, but it’s hard to tell that from looking at the code locally so long term it’s not a great design pattern.
Comment 12 stefan.meschke 2021-11-12 00:34:38 PST
Is there already an ETA when this will be released as a fix for iOS 15? Seems like iOS 15.1 nor iOS 15.2 (public beta) got this. Thanks in advance!
Comment 13 Wenson Hsieh 2021-11-12 08:00:08 PST
(In reply to stefan.meschke from comment #12)
> Is there already an ETA when this will be released as a fix for iOS 15?
> Seems like iOS 15.1 nor iOS 15.2 (public beta) got this. Thanks in advance!

So it looks like this change didn't make it into iOS 15.2 beta 2 (19C5036e). We'll update the bug when it lands in a public beta!
Comment 14 stefan.meschke 2021-11-12 09:01:21 PST
(In reply to Wenson Hsieh from comment #13)
> (In reply to stefan.meschke from comment #12)
> > Is there already an ETA when this will be released as a fix for iOS 15?
> > Seems like iOS 15.1 nor iOS 15.2 (public beta) got this. Thanks in advance!
> 
> So it looks like this change didn't make it into iOS 15.2 beta 2 (19C5036e).
> We'll update the bug when it lands in a public beta!

Perfect. Thank you!
Comment 15 shakil mansuri 2021-12-01 21:10:06 PST
This issue still exists with iOS 15.1.

You can see https://bug-231161-attachments.webkit.org/attachment.cgi?id=440044.
Comment 16 Tina 2022-04-14 14:47:22 PDT
May I ask when will this bug be verified please?
Comment 17 Darin Adler 2022-04-14 15:10:31 PDT
Sorry no one at Apple mentioned it when it made it into a public beta. This fix did make it into iOS 15.3. The current version of iOS, iOS 15.4 should also have it fixed. Feel free to verify it in iOS 15.3 or newer.
Comment 18 Darin Adler 2022-04-14 15:12:42 PDT
I believe it also made it into the final version of iOS 15.2, now that I look at Apple’s internal records more carefully.