Bug 224683 – [macOS] Add some support for webpage translation in WebKitLegacy

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

<rdar://problem/75641882>

[Wenson Hsieh]

Created attachment 426262 [details]
Blocked on #224680

[Wenson Hsieh]

Created attachment 426269 [details]
Blocked on #224680

[Wenson Hsieh]

Comment on attachment 426269 [details]
Blocked on #224680

(This is still blocked on the other bug — I'm going to retry all EWS once it's able to apply)

[Wenson Hsieh]

Created attachment 426295 [details]
Fix iOS 14/Big Sur builds

[Wenson Hsieh]

Created attachment 426299 [details]
Fix iOS 14/Big Sur builds (2)

[Darin Adler]

Comment on attachment 426299 [details]
Fix iOS 14/Big Sur builds (2)

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

> Source/WebKitLegacy/mac/WebView/WebView.mm:9652
> +    auto textToTranslate = adoptNS([[NSAttributedString alloc] initWithString:text]);

We don’t need this local variable. Could just merge this line with the next.

> Source/WebKitLegacy/mac/WebView/WebView.mm:9655
> +        [translationViewController setPreferredContentSize:NSMakeSize(400, 400)];

Hardcoded magic size without comment is not great.

> Source/WebKitLegacy/mac/WebView/WebView.mm:9670
> +    if (aim == highlight)

Seems wrong to ever compare two floating point values and expect them to be exactly equal. Seems to me this should be some other kind of check, perhaps a check that the absolute value is one pixel or more.

[Darin Adler]

Comment on attachment 426299 [details]
Fix iOS 14/Big Sur builds (2)

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

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9670
>> +    if (aim == highlight)
> 
> Seems wrong to ever compare two floating point values and expect them to be exactly equal. Seems to me this should be some other kind of check, perhaps a check that the absolute value is one pixel or more.

Over some threshold. Not sure "1 pixel" is quite right. I don’t really understand the preferred edge heuristic, which is probably why I can’t make a good suggestion.

[Wenson Hsieh]

Comment on attachment 426299 [details]
Fix iOS 14/Big Sur builds (2)

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

Thanks for the review!

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9652
>> +    auto textToTranslate = adoptNS([[NSAttributedString alloc] initWithString:text]);
> 
> We don’t need this local variable. Could just merge this line with the next.

Fixed!

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9655
>> +        [translationViewController setPreferredContentSize:NSMakeSize(400, 400)];
> 
> Hardcoded magic size without comment is not great.

Oh, this reminded me that on recent builds of macOS, this fallback size should no longer be necessary. I removed these two lines.

>>> Source/WebKitLegacy/mac/WebView/WebView.mm:9670
>>> +    if (aim == highlight)
>> 
>> Seems wrong to ever compare two floating point values and expect them to be exactly equal. Seems to me this should be some other kind of check, perhaps a check that the absolute value is one pixel or more.
> 
> Over some threshold. Not sure "1 pixel" is quite right. I don’t really understand the preferred edge heuristic, which is probably why I can’t make a good suggestion.

So I actually took this part directly from the Reveal framework's logic for setting up and presenting `LTUITranslationViewController`. I believe the intent here is to consult the `userInterfaceLayoutDirection` as a sort of tiebreaker in the case where the menu location is exactly in the middle of the bounding rect of the selection, but in practice, it's pretty unlikely that we'll end up in this codepath.

I'll change this to use `areEssentiallyEqual` instead, since that's what we generally use to (sensibly) compare floating point values elsewhere in WebKit.

[Darin Adler]

Comment on attachment 426299 [details]
Fix iOS 14/Big Sur builds (2)

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

>>>> Source/WebKitLegacy/mac/WebView/WebView.mm:9670
>>>> +    if (aim == highlight)
>>> 
>>> Seems wrong to ever compare two floating point values and expect them to be exactly equal. Seems to me this should be some other kind of check, perhaps a check that the absolute value is one pixel or more.
>> 
>> Over some threshold. Not sure "1 pixel" is quite right. I don’t really understand the preferred edge heuristic, which is probably why I can’t make a good suggestion.
> 
> So I actually took this part directly from the Reveal framework's logic for setting up and presenting `LTUITranslationViewController`. I believe the intent here is to consult the `userInterfaceLayoutDirection` as a sort of tiebreaker in the case where the menu location is exactly in the middle of the bounding rect of the selection, but in practice, it's pretty unlikely that we'll end up in this codepath.
> 
> I'll change this to use `areEssentiallyEqual` instead, since that's what we generally use to (sensibly) compare floating point values elsewhere in WebKit.

That’s OK, and better but it’s still not logical, even though the Reveal framework does it. Obviously we want to share the same logic as the rest of the OS, so it’s not important to do better and would be best to match as well as we can, ideally sharing the code.

A tie breaker used when a person happens to click super-precisely seems obviously wrong, not logical. The saving grace, I suppose, is that as you say it’s so unlikely that it’s nearly irrelevant. This is essentially dead code.

[Wenson Hsieh]

Created attachment 426365 [details]
Patch for landing

[EWS]

Committed r276220 (236702@main): <https://commits.webkit.org/236702@main>

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