Bug 221636 - [iOS][FCR] Add new picker for <select multiple> elements
Summary: [iOS][FCR] Add new picker for <select multiple> elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-09 15:04 PST by Aditya Keerthi
Modified: 2021-02-11 14:25 PST (History)
4 users (show)

See Also:


Attachments
Patch (28.55 KB, patch)
2021-02-09 15:07 PST, Aditya Keerthi
wenson_hsieh: review+
Details | Formatted Diff | Diff
Patch for landing (28.88 KB, patch)
2021-02-11 11:24 PST, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2021-02-09 15:04:19 PST
...
Comment 1 Aditya Keerthi 2021-02-09 15:04:36 PST
<rdar://problem/72399337>
Comment 2 Aditya Keerthi 2021-02-09 15:07:11 PST
Created attachment 419772 [details]
Patch
Comment 3 Wenson Hsieh 2021-02-11 09:49:20 PST
Comment on attachment 419772 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:733
> +    RetainPtr<UIBarButtonItem> nextPreviousSpacer = adoptNS([[UIBarButtonItem alloc] initWithBarButtonSystemItem:UIBarButtonSystemItemFixedSpace target:nil action:NULL]);

Nit - I think `auto` instead of `RetainPtr<UIBarButtonItem>` would be a bit cleaner here, for the `nextPreviousSpacer` and `closeButton`.

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:744
> +    for (const OptionItem& option : _contentView.focusedSelectElementOptions) {

Nit - `auto& option`?

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:763
> +    for (const OptionItem& option : _contentView.focusedSelectElementOptions) {

Nit - `auto& option`?

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:815
> +    for (const OptionItem& option : _contentView.focusedSelectElementOptions) {

Nit - auto&?

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:903
> +    for (const OptionItem& option : _contentView.focusedSelectElementOptions) {

Nit - `auto& option`?

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:919
> +- (OptionItem *)optionItemAtIndexPath:(NSIndexPath *)indexPath

Nit - this is fine as-is since all call sites use the `OptionItem` immediately without risk of reentrant calls that might change the focused element options, but I think it would be safer to instead return an `Optional<OptionItem>` instead. While this does copy `OptionItem` an extra time, it would avoid any risk of accidentally UAF-ing option items in the future.

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:922
> +    for (OptionItem& option : _contentView.focusedSelectElementOptions) {

Nit - `auto& option`?

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:950
> +    ASSERT(option);

Nit - I think this is okay as-is since there are no out-of-band IPC messages a compromised web process could use to update focused element from underneath us, but it might still be safer to add a null check here.

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:958
> +    if (option->disabled)
> +        cell.userInteractionEnabled = NO;
> +    else
> +        cell.userInteractionEnabled = YES;

Nit - this might be cleaner as `cell.userInteractionEnabled = !option->disabled;`

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:979
> +    ASSERT(option);

(Ditto)

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:993
> +        [_contentView accessoryTab:YES];

These should probably capture the content view (either weakly or strongly).

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:1000
> +        [_contentView accessoryTab:NO];

(Ditto)

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:1007
> +        [_contentView accessoryDone];

(Ditto)

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:1095
> +    NSInteger currentRow = -1;
> +    NSInteger totalRows = -1;

Nit - can we instead start these at 0, and move the increment after the return statement below?

> Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:1097
> +    for (const OptionItem& option : _view.focusedSelectElementOptions) {

Nit - `auto& option`?
Comment 4 Aditya Keerthi 2021-02-11 11:24:49 PST
Created attachment 420010 [details]
Patch for landing
Comment 5 EWS 2021-02-11 14:25:34 PST
Committed r272753: <https://commits.webkit.org/r272753>

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