Bug 230205 - [macOS] -toggleAutomaticSpellingCorrection: menu item validation shouldn't require post-layout editor state
Summary: [macOS] -toggleAutomaticSpellingCorrection: menu item validation shouldn't re...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-12 11:53 PDT by Wenson Hsieh
Modified: 2021-09-13 15:50 PDT (History)
9 users (show)

See Also:


Attachments
For EWS (17.70 KB, patch)
2021-09-12 12:40 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix iOS build (17.21 KB, patch)
2021-09-12 13:04 PDT, Wenson Hsieh
darin: review+
Details | Formatted Diff | Diff
For landing (17.25 KB, patch)
2021-09-13 13:56 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 Wenson Hsieh 2021-09-12 11:53:32 PDT
.
Comment 1 Wenson Hsieh 2021-09-12 12:40:15 PDT
Created attachment 437994 [details]
For EWS
Comment 2 Wenson Hsieh 2021-09-12 13:04:55 PDT
Created attachment 437995 [details]
Fix iOS build
Comment 3 Darin Adler 2021-09-13 13:28:12 PDT
Comment on attachment 437995 [details]
Fix iOS build

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

> Source/WebCore/editing/AlternativeTextController.cpp:348
> +    } else if (RefPtr editableRoot = position.rootEditableElement()) {
> +        if (is<HTMLElement>(editableRoot) && !downcast<HTMLElement>(*editableRoot).shouldAutocorrect())
> +            return false;
>      }

This is null checking twice. The old code was doing that too. There are a couple different ways to avoid that. Here’s one:

    } else if (RefPtr editableRoot = position.rootEditableElement(); is<HTMLElement>(editableRoot)) {

Here’s another:

    } else if (RefPtr editableRoot = position.rootEditableElement()) {
        if (is<HTMLElement>(*editableRoot) && !downcast<HTMLElement>(*editableRoot).shouldAutocorrect())
            return false;
    }
Comment 4 Wenson Hsieh 2021-09-13 13:37:13 PDT
Comment on attachment 437995 [details]
Fix iOS build

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

Thanks for the review!

>> Source/WebCore/editing/AlternativeTextController.cpp:348
>>      }
> 
> This is null checking twice. The old code was doing that too. There are a couple different ways to avoid that. Here’s one:
> 
>     } else if (RefPtr editableRoot = position.rootEditableElement(); is<HTMLElement>(editableRoot)) {
> 
> Here’s another:
> 
>     } else if (RefPtr editableRoot = position.rootEditableElement()) {
>         if (is<HTMLElement>(*editableRoot) && !downcast<HTMLElement>(*editableRoot).shouldAutocorrect())
>             return false;
>     }

Good catch — I'll go with the first option since it's a bit more succinct, and also move the `!downcast<HTMLElement>(*editableRoot).shouldAutocorrect()` next to the type check.
Comment 5 Wenson Hsieh 2021-09-13 13:56:57 PDT
Created attachment 438075 [details]
For landing
Comment 6 EWS 2021-09-13 15:49:16 PDT
Committed r282370 (241633@main): <https://commits.webkit.org/241633@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438075 [details].
Comment 7 Radar WebKit Bug Importer 2021-09-13 15:50:16 PDT
<rdar://problem/83076301>