Changeset 261815 in webkit

Timestamp:

May 18, 2020 9:23:30 AM (4 years ago)

Author:

Wenson Hsieh

Message:

Single selection <select> with <optgroups> shows multiple selected options
​https://bugs.webkit.org/show_bug.cgi?id=199485
<rdar://problem/52757531>

Reviewed by Megan Gardner.

Source/WebKit:

Fixes a long-standing bug in WKMultipleSelectPicker. Prior to this patch, we rely on the delegate method
-pickerView:row:column:checked: to be called twice whenever an item is selected: one time for the item that is
no longer checked, and another for the newly checked item. This method is responsible for updating the cached
FocusedElementInformation that determines the data model for the select menu, with the expectation that the
unchecked item would be updated to have isSelected = false;, and the new checked item would have isSelected
= true;.

However, -pickerView:row:column:checked: is only called for visible item cells. This means that if the user
checks an item, scrolls the select menu items down so that the checked item is offscreen, and then checks a
different item, we only get notified that the new item is checked, and as a result, fail to uncheck the previous
item.

To address this, tweak our logic for handling a single select so that when an item is checked, we additionally
update the previously checked item to not be selected. Also, fix what seems to be a bug in the logic for
updating _singleSelectionIndex, which is currently updated even when the item is unchecked. It seems to work
out at the moment, because -pickerView:row:column:checked: seems to be called with checked := YES after the
previous item was unchecked (assuming that it was visible).

Test: fast/forms/ios/no-stale-checked-items-in-select-picker.html

• UIProcess/API/ios/WKWebViewPrivateForTestingIOS.h:
• UIProcess/API/ios/WKWebViewTestingIOS.mm:

(-[WKWebView selectFormAccessoryHasCheckedItemAtRow:]):

• UIProcess/ios/WKContentViewInteraction.h:
• UIProcess/ios/WKContentViewInteraction.mm:

(-[WKContentView selectFormAccessoryHasCheckedItemAtRow:]):

Add plumbing for a new testing hook.

• UIProcess/ios/forms/WKFormSelectControl.h:
• UIProcess/ios/forms/WKFormSelectControl.mm:

(-[WKFormSelectControl selectFormAccessoryHasCheckedItemAtRow:]):

• UIProcess/ios/forms/WKFormSelectPicker.mm:

(-[WKMultipleSelectPicker pickerView:viewForRow:forComponent:reusingView:]):
(-[WKMultipleSelectPicker pickerView:row:column:checked:]):
(-[WKMultipleSelectPicker selectRow:inComponent:extendingSelection:]):

Also, fix an existing bug in this testing helper method that crashed the test runner due to calling an
unimplemented selector. Instead of trying to invoke -pickerView:didSelectRow:inComponent:, we should be using
-pickerView:row:column:checked: instead for multiple select pickers (which, somewhat confusingly, are still
used for single select elements that have optgroups.)

(-[WKMultipleSelectPicker selectFormAccessoryHasCheckedItemAtRow:]):

Tools:

Add a new helper method to check whether the currently presented form accessory is a select menu, and has a
checked menu item at the given row.

• TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:
• TestRunnerShared/UIScriptContext/UIScriptController.h:

(WTR::UIScriptController::selectFormAccessoryHasCheckedItemAtRow const):

• WebKitTestRunner/ios/UIScriptControllerIOS.h:
• WebKitTestRunner/ios/UIScriptControllerIOS.mm:

(WTR::UIScriptControllerIOS::selectFormAccessoryHasCheckedItemAtRow const):

LayoutTests:

Add a layout test to verify that we don't leave behind a checked select item after scrolling it offscreen and
then checking a different item.

• fast/forms/ios/no-stale-checked-items-in-select-picker-expected.txt: Added.
• fast/forms/ios/no-stale-checked-items-in-select-picker.html: Added.
• platform/ipad/TestExpectations:
• resources/ui-helper.js:

(window.UIHelper.selectFormAccessoryPickerRow):
(window.UIHelper.selectFormAccessoryHasCheckedItemAtRow):

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/forms/ios/no-stale-checked-items-in-select-picker-expected.txt (added)
• LayoutTests/fast/forms/ios/no-stale-checked-items-in-select-picker.html (added)
• LayoutTests/platform/ipad/TestExpectations (modified) (1 diff)
• LayoutTests/resources/ui-helper.js (modified) (1 diff)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/API/ios/WKWebViewPrivateForTestingIOS.h (modified) (1 diff)
• Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm (modified) (1 diff)
• Source/WebKit/UIProcess/ios/WKContentViewInteraction.h (modified) (1 diff)
• Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm (modified) (1 diff)
• Source/WebKit/UIProcess/ios/forms/WKFormSelectControl.h (modified) (2 diffs)
• Source/WebKit/UIProcess/ios/forms/WKFormSelectControl.mm (modified) (1 diff)
• Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm (modified) (4 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl (modified) (1 diff)
• Tools/TestRunnerShared/UIScriptContext/UIScriptController.h (modified) (1 diff)
• Tools/WebKitTestRunner/ios/UIScriptControllerIOS.h (modified) (1 diff)
• Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2020-05-18  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Single selection <select> with <optgroups> shows multiple selected options
+        https://bugs.webkit.org/show_bug.cgi?id=199485
+        <rdar://problem/52757531>
+
+        Reviewed by Megan Gardner.
+
+        Add a layout test to verify that we don't leave behind a checked select item after scrolling it offscreen and
+        then checking a different item.
+
+        * fast/forms/ios/no-stale-checked-items-in-select-picker-expected.txt: Added.
+        * fast/forms/ios/no-stale-checked-items-in-select-picker.html: Added.
+        * platform/ipad/TestExpectations:
+        * resources/ui-helper.js:
+        (window.UIHelper.selectFormAccessoryPickerRow):
+        (window.UIHelper.selectFormAccessoryHasCheckedItemAtRow):
+
 2020-05-18  Youenn Fablet  <youenn@apple.com>
 

trunk/LayoutTests/platform/ipad/TestExpectations

@@ -8 +8 @@
 fast/forms/ios/user-scalable-scales-for-keyboard-focus-with-no-author-defined-scale.html [ Skip ]
 fast/visual-viewport/ios/caret-after-focus-in-fixed.html [ Skip ]
+
+# The select picker input view is not displayed on iPad.
+fast/forms/ios/no-stale-checked-items-in-select-picker.html
 
 # These tests are designed for iPhone and crash on iPad

trunk/LayoutTests/resources/ui-helper.js

@@ -723 +723 @@
     static selectFormAccessoryPickerRow(rowIndex)
     {
-        const selectRowScript = `(() => uiController.selectFormAccessoryPickerRow(${rowIndex}))()`;
+        const selectRowScript = `uiController.selectFormAccessoryPickerRow(${rowIndex})`;
         return new Promise(resolve => testRunner.runUIScript(selectRowScript, resolve));
+    }
+
+    static selectFormAccessoryHasCheckedItemAtRow(rowIndex)
+    {
+        return new Promise(resolve => testRunner.runUIScript(`uiController.selectFormAccessoryHasCheckedItemAtRow(${rowIndex})`, result => {
+            resolve(result === "true");
+        }));
     }
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2020-05-18  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Single selection <select> with <optgroups> shows multiple selected options
+        https://bugs.webkit.org/show_bug.cgi?id=199485
+        <rdar://problem/52757531>
+
+        Reviewed by Megan Gardner.
+
+        Fixes a long-standing bug in WKMultipleSelectPicker. Prior to this patch, we rely on the delegate method
+        `-pickerView:row:column:checked:` to be called twice whenever an item is selected: one time for the item that is
+        no longer checked, and another for the newly checked item. This method is responsible for updating the cached
+        `FocusedElementInformation` that determines the data model for the select menu, with the expectation that the
+        unchecked item would be updated to have `isSelected = false;`, and the new checked item would have `isSelected`
+        `= true;`.
+
+        However, `-pickerView:row:column:checked:` is only called for visible item cells. This means that if the user
+        checks an item, scrolls the select menu items down so that the checked item is offscreen, and then checks a
+        different item, we only get notified that the new item is checked, and as a result, fail to uncheck the previous
+        item.
+
+        To address this, tweak our logic for handling a single select so that when an item is checked, we additionally
+        update the previously checked item to not be selected. Also, fix what seems to be a bug in the logic for
+        updating `_singleSelectionIndex`, which is currently updated even when the item is unchecked. It seems to work
+        out at the moment, because `-pickerView:row:column:checked:` seems to be called with `checked := YES` after the
+        previous item was unchecked (assuming that it was visible).
+
+        Test: fast/forms/ios/no-stale-checked-items-in-select-picker.html
+
+        * UIProcess/API/ios/WKWebViewPrivateForTestingIOS.h:
+        * UIProcess/API/ios/WKWebViewTestingIOS.mm:
+        (-[WKWebView selectFormAccessoryHasCheckedItemAtRow:]):
+        * UIProcess/ios/WKContentViewInteraction.h:
+        * UIProcess/ios/WKContentViewInteraction.mm:
+        (-[WKContentView selectFormAccessoryHasCheckedItemAtRow:]):
+
+        Add plumbing for a new testing hook.
+
+        * UIProcess/ios/forms/WKFormSelectControl.h:
+        * UIProcess/ios/forms/WKFormSelectControl.mm:
+        (-[WKFormSelectControl selectFormAccessoryHasCheckedItemAtRow:]):
+        * UIProcess/ios/forms/WKFormSelectPicker.mm:
+        (-[WKMultipleSelectPicker pickerView:viewForRow:forComponent:reusingView:]):
+        (-[WKMultipleSelectPicker pickerView:row:column:checked:]):
+        (-[WKMultipleSelectPicker selectRow:inComponent:extendingSelection:]):
+
+        Also, fix an existing bug in this testing helper method that crashed the test runner due to calling an
+        unimplemented selector. Instead of trying to invoke `-pickerView:didSelectRow:inComponent:`, we should be using
+        `-pickerView:row:column:checked:` instead for multiple select pickers (which, somewhat confusingly, are still
+        used for single select elements that have `optgroup`s.)
+
+        (-[WKMultipleSelectPicker selectFormAccessoryHasCheckedItemAtRow:]):
+
 2020-05-18  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebKit/UIProcess/API/ios/WKWebViewPrivateForTestingIOS.h

@@ -49 +49 @@
 - (void)_dismissFilePicker;
 - (void)selectFormAccessoryPickerRow:(int)rowIndex;
+- (BOOL)selectFormAccessoryHasCheckedItemAtRow:(long)rowIndex;
 
 - (BOOL)_mayContainEditableElementsInRect:(CGRect)rect;

trunk/Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm

@@ -145 +145 @@
 }
 
+- (BOOL)selectFormAccessoryHasCheckedItemAtRow:(long)rowIndex
+{
+    return [_contentView selectFormAccessoryHasCheckedItemAtRow:rowIndex];
+}
+
 - (NSString *)selectFormPopoverTitle
 {

trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.h

@@ -620 +620 @@
 - (void)_simulateTextEntered:(NSString *)text;
 - (void)selectFormAccessoryPickerRow:(NSInteger)rowIndex;
+- (BOOL)selectFormAccessoryHasCheckedItemAtRow:(long)rowIndex;
 - (void)setTimePickerValueToHour:(NSInteger)hour minute:(NSInteger)minute;
 - (double)timePickerValueHour;

trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm

@@ -8758 +8758 @@
 }
 
+- (BOOL)selectFormAccessoryHasCheckedItemAtRow:(long)rowIndex
+{
+#if !PLATFORM(WATCHOS)
+    if ([_inputPeripheral isKindOfClass:[WKFormSelectControl self]])
+        return [(WKFormSelectControl *)_inputPeripheral selectFormAccessoryHasCheckedItemAtRow:rowIndex];
+#endif
+    return NO;
+}
+
 - (NSString *)textContentTypeForTesting
 {

trunk/Source/WebKit/UIProcess/ios/forms/WKFormSelectControl.h

@@ -42 +42 @@
 @interface WKFormSelectControl(WKTesting)
 - (void)selectRow:(NSInteger)rowIndex inComponent:(NSInteger)componentIndex extendingSelection:(BOOL)extendingSelection;
+- (BOOL)selectFormAccessoryHasCheckedItemAtRow:(long)rowIndex;
 @property (nonatomic, readonly) NSString *selectFormPopoverTitle;
 @end
@@ -48 +49 @@
 @optional
 - (void)selectRow:(NSInteger)rowIndex inComponent:(NSInteger)componentIndex extendingSelection:(BOOL)extendingSelection;
+- (BOOL)selectFormAccessoryHasCheckedItemAtRow:(long)rowIndex;
 @end
 

trunk/Source/WebKit/UIProcess/ios/forms/WKFormSelectControl.mm

@@ -102 +102 @@
 }
 
+- (BOOL)selectFormAccessoryHasCheckedItemAtRow:(long)rowIndex
+{
+    return [self.control respondsToSelector:@selector(selectFormAccessoryHasCheckedItemAtRow:)]
+        && [id<WKSelectTesting>(self.control) selectFormAccessoryHasCheckedItemAtRow:rowIndex];
+}
+
 @end
 

trunk/Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm

@@ -220 +220 @@
 - (UIView *)pickerView:(UIPickerView *)pickerView viewForRow:(NSInteger)rowIndex forComponent:(NSInteger)columnIndex reusingView:(UIView *)view
 {
-    const OptionItem& item = [_view focusedSelectElementOptions][rowIndex];
-    UIPickerContentView* pickerItem = item.isGroup ? [[[WKOptionGroupPickerCell alloc] initWithOptionItem:item] autorelease] : [[[WKOptionPickerCell alloc] initWithOptionItem:item] autorelease];
+    auto& item = [_view focusedSelectElementOptions][rowIndex];
+    UIPickerContentView *pickerItem = item.isGroup ? [[[WKOptionGroupPickerCell alloc] initWithOptionItem:item] autorelease] : [[[WKOptionPickerCell alloc] initWithOptionItem:item] autorelease];
 
     // The cell starts out with a null frame. We need to set its frame now so we can find the right font size.
@@ -273 +273 @@
 - (void)pickerView:(UIPickerView *)pickerView row:(int)rowIndex column:(int)columnIndex checked:(BOOL)isChecked
 {
-    if ((size_t)rowIndex >= [_view focusedSelectElementOptions].size())
+    auto numberOfOptions = static_cast<NSUInteger>([_view focusedSelectElementOptions].size());
+    if (numberOfOptions <= static_cast<NSUInteger>(rowIndex))
         return;
 
-    OptionItem& item = [_view focusedSelectElementOptions][rowIndex];
+    auto& item = [_view focusedSelectElementOptions][rowIndex];
 
     // FIXME: Remove this workaround once <rdar://problem/18745253> is fixed.
@@ -292 +293 @@
         [_view page]->setFocusedElementSelectedIndex([self findItemIndexAt:rowIndex], true);
         item.isSelected = isChecked;
-    } else {
+    } else if (isChecked) {
         // Single selection.
-        item.isSelected = NO;
+        if (_singleSelectionIndex < numberOfOptions)
+            [_view focusedSelectElementOptions][_singleSelectionIndex].isSelected = false;
+
         _singleSelectionIndex = rowIndex;
 
         // This private delegate often gets called for multiple rows in the picker,
         // so we only activate and set as selected the checked item in single selection.
-        if (isChecked) {
-            [_view page]->setFocusedElementSelectedIndex([self findItemIndexAt:rowIndex]);
-            item.isSelected = YES;
-        }
-    }
+        [_view page]->setFocusedElementSelectedIndex([self findItemIndexAt:rowIndex]);
+        item.isSelected = true;
+    } else
+        item.isSelected = false;
 }
 
@@ -312 +314 @@
     [self selectRow:rowIndex inComponent:0 animated:NO];
     // Progammatic selection changes don't call the delegate, so do that manually.
-    [self.delegate pickerView:self didSelectRow:rowIndex inComponent:0];
+    [self pickerView:self row:rowIndex column:0 checked:YES];
+}
+
+- (BOOL)selectFormAccessoryHasCheckedItemAtRow:(long)rowIndex
+{
+    auto numberOfRows = [self numberOfRowsInComponent:0];
+    if (rowIndex >= numberOfRows)
+        return NO;
+
+    return [(UIPickerContentView *)[self viewForRow:rowIndex forComponent:0] isChecked];
 }
 

trunk/Tools/ChangeLog

@@ +1 @@
+2020-05-18  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Single selection <select> with <optgroups> shows multiple selected options
+        https://bugs.webkit.org/show_bug.cgi?id=199485
+        <rdar://problem/52757531>
+
+        Reviewed by Megan Gardner.
+
+        Add a new helper method to check whether the currently presented form accessory is a select menu, and has a
+        checked menu item at the given row.
+
+        * TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:
+        * TestRunnerShared/UIScriptContext/UIScriptController.h:
+        (WTR::UIScriptController::selectFormAccessoryHasCheckedItemAtRow const):
+        * WebKitTestRunner/ios/UIScriptControllerIOS.h:
+        * WebKitTestRunner/ios/UIScriptControllerIOS.mm:
+        (WTR::UIScriptControllerIOS::selectFormAccessoryHasCheckedItemAtRow const):
+
 2020-05-18  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl

@@ -213 +213 @@
     // <select> picker
     void selectFormAccessoryPickerRow(long rowIndex);
+    boolean selectFormAccessoryHasCheckedItemAtRow(long rowIndex);
     readonly attribute DOMString selectFormPopoverTitle;
     readonly attribute DOMString formInputLabel;

trunk/Tools/TestRunnerShared/UIScriptContext/UIScriptController.h

@@ -206 +206 @@
     virtual void dismissFormAccessoryView() { notImplemented(); }
     virtual void selectFormAccessoryPickerRow(long) { notImplemented(); }
+    virtual bool selectFormAccessoryHasCheckedItemAtRow(long) const { return false; }
     virtual JSRetainPtr<JSStringRef> textContentType() const { notImplemented(); return nullptr; }
     virtual JSRetainPtr<JSStringRef> selectFormPopoverTitle() const { notImplemented(); return nullptr; }

trunk/Tools/WebKitTestRunner/ios/UIScriptControllerIOS.h

@@ -89 +89 @@
     JSRetainPtr<JSStringRef> formInputLabel() const override;
     void selectFormAccessoryPickerRow(long rowIndex) override;
+    bool selectFormAccessoryHasCheckedItemAtRow(long rowIndex) const override;
     void setTimePickerValue(long hour, long minute) override;
     double timePickerValueHour() const override;

trunk/Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm

@@ -607 +607 @@
 }
 
+bool UIScriptControllerIOS::selectFormAccessoryHasCheckedItemAtRow(long rowIndex) const
+{
+    return [webView() selectFormAccessoryHasCheckedItemAtRow:rowIndex];
+}
+
 void UIScriptControllerIOS::setTimePickerValue(long hour, long minute)
 {