Skip to content

Commit

Permalink
[iOS] <option> elements outside of an <optgroup> are added to the pre…
Browse files Browse the repository at this point in the history
…ceding group

https://bugs.webkit.org/show_bug.cgi?id=264192
rdar://117930480

Reviewed by Wenson Hsieh.

<option> elements are displayed in the UI process by sending over a vector of
options that includes a minimal data representation of <optgroup> and <option>
elements. Currently, <options> are associated with an <optgroup> by incrementing
a counter whenever an <optgroup> is encountered, and then storing the current
counter value on the <option> representation.

The current approach is flawed when an <optgroup> terminates, and is followed by
<option> elements outside of a group. In this case, the counter is not incremented,
and the options are associated with the preceding group.

To fix, keep track of the current <optgroup> element, and when an option that no
longer belongs to the current <optgroup> is encountered, insert a "fake" group and
increment the counter. This approach ensures that trailing <options> do not appear
as part of the preceding group. This fix is not applied for the picker wheel UI
used in bincompat scenarios, as the UI does not allow for ungrouped options to be
distinguished.

Additionally, fix the appearance of groups with no items and groups missing a title,
to exclude a disclosure button, and remove padding, respectively.

* LayoutTests/fast/forms/ios/select-multiple-options-after-optgroup-expected.txt: Added.
* LayoutTests/fast/forms/ios/select-multiple-options-after-optgroup.html: Added.
* LayoutTests/fast/forms/ios/select-options-after-optgroup-expected.txt: Added.
* LayoutTests/fast/forms/ios/select-options-after-optgroup.html: Added.
* Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm:
(-[WKSelectPicker createMenu]):
(-[WKSelectPickerGroupHeaderView initWithGroupName:section:isCollapsible:]):
(-[WKSelectPickerGroupHeaderView setCollapsed:animated:]):
(-[WKSelectPickerTableViewController tableView:heightForHeaderInSection:]):

Groups without a title should appear as a smaller amount of padding, rather
than a full size header.

(-[WKSelectPickerTableViewController tableView:viewForHeaderInSection:]):

Empty groups are not collapsible, and should not show a disclosure button.

(-[WKSelectPickerTableViewController didTapSelectPickerGroupHeaderView:]):
(-[WKSelectPickerGroupHeaderView initWithGroupName:section:]): Deleted.
* Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::WebPage::focusedElementInformation):

Keep track of the current <optgroup> element when one is encountered. If the
next <option> does not belong to the same group, make it part of a new group
with an empty title.

Canonical link: https://commits.webkit.org/270235@main
  • Loading branch information
pxlcoder committed Nov 5, 2023
1 parent 671b617 commit 4d943d6
Show file tree
Hide file tree
Showing 6 changed files with 197 additions and 29 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
This test verifies that choosing multiple options which have preceding optgroup siblings works as expected.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


PASS selectValues is ""
PASS selectValues is "B"
PASS selectValues is "B,2"
PASS selectValues is "B,2,D"
PASS selectValues is "B,2,D,E"
PASS successfullyParsed is true

TEST COMPLETE

Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
<html>
<head>
<meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
<script src="../../../resources/js-test.js"></script>
<script src="../../../resources/ui-helper.js"></script>
</head>
<body>
<select multiple id="select">
<option>A</option>
<option>B</option>
<option>C</option>
<optgroup label="Numbers">
<option>1</option>
<option>2</option>
<option>3</option>
</optgroup>
<option>D</option>
<option>E</option>
</select>
</body>
<script>
jsTestIsAsync = true;

function valuesForSelectElement(element)
{
return Array.from(element.selectedOptions).map(o => o.value).toString();
}

async function selectRowAndAssertValues(row, values)
{
await UIHelper.ensurePresentationUpdate();
await UIHelper.selectFormAccessoryPickerRow(row);
await UIHelper.ensurePresentationUpdate();

selectValues = valuesForSelectElement(select);
shouldBeEqualToString("selectValues", values);
}

addEventListener("load", async () => {
description("This test verifies that choosing multiple options which have preceding optgroup siblings works as expected.");

selectValues = valuesForSelectElement(select);
shouldBeEqualToString("selectValues", "");

await UIHelper.activateElement(select);
await selectRowAndAssertValues(1, "B");
await selectRowAndAssertValues(4, "B,2");
await selectRowAndAssertValues(6, "B,2,D");
await selectRowAndAssertValues(7, "B,2,D,E");

finishJSTest();
});
</script>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
This test verifies that choosing options which have preceding optgroup siblings works as expected.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


PASS select.value is "A"
PASS select.value is "B"
PASS select.value is "2"
PASS select.value is "D"
PASS successfullyParsed is true

TEST COMPLETE

51 changes: 51 additions & 0 deletions LayoutTests/fast/forms/ios/select-options-after-optgroup.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
<html>
<head>
<meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
<script src="../../../resources/js-test.js"></script>
<script src="../../../resources/ui-helper.js"></script>
</head>
<body>
<select id="select">
<option>A</option>
<option>B</option>
<option>C</option>
<optgroup label="Numbers">
<option>1</option>
<option>2</option>
<option>3</option>
</optgroup>
<option>D</option>
<option>E</option>
</select>
</body>
<script>
jsTestIsAsync = true;

addEventListener("load", async () => {
description("This test verifies that choosing options which have preceding optgroup siblings works as expected.");

shouldBeEqualToString("select.value", "A");

await UIHelper.activateElement(select);
await UIHelper.waitForContextMenuToShow();
await UIHelper.selectFormAccessoryPickerRow(1);
await UIHelper.waitForContextMenuToHide();
shouldBeEqualToString("select.value", "B");

await UIHelper.activateElement(select);
await UIHelper.waitForContextMenuToShow();
await UIHelper.selectFormAccessoryPickerRow(4);
await UIHelper.waitForContextMenuToHide();
shouldBeEqualToString("select.value", "2");

await UIHelper.activateElement(select);
await UIHelper.waitForContextMenuToShow();
await UIHelper.selectFormAccessoryPickerRow(6);
await UIHelper.waitForContextMenuToHide();
shouldBeEqualToString("select.value", "D");

finishJSTest();
});
</script>
</html>
68 changes: 45 additions & 23 deletions Source/WebKit/UIProcess/ios/forms/WKFormSelectPicker.mm
Original file line number Diff line number Diff line change
Expand Up @@ -574,13 +574,14 @@ - (UIMenu *)createMenu
while (currentIndex < _view.focusedSelectElementOptions.size()) {
auto& optionItem = _view.focusedSelectElementOptions[currentIndex];
if (optionItem.isGroup) {
auto groupID = optionItem.parentGroupID;
NSString *groupText = optionItem.text;
NSMutableArray *groupedItems = [NSMutableArray array];

currentIndex++;
while (currentIndex < _view.focusedSelectElementOptions.size()) {
auto& childOptionItem = _view.focusedSelectElementOptions[currentIndex];
if (childOptionItem.isGroup)
if (childOptionItem.isGroup || childOptionItem.parentGroupID != groupID)
break;

UIAction *action = [self actionForOptionItem:childOptionItem withIndex:optionIndex];
Expand Down Expand Up @@ -754,6 +755,7 @@ - (BOOL)selectFormAccessoryHasCheckedItemAtRow:(long)rowIndex

@interface WKSelectPickerGroupHeaderView : UIView
@property (nonatomic, readonly) NSInteger section;
@property (nonatomic, readonly) BOOL isCollapsible;
@end

@interface WKSelectPickerTableViewController : UITableViewController
Expand All @@ -773,12 +775,13 @@ @implementation WKSelectPickerGroupHeaderView {
BOOL _collapsed;
}

- (instancetype)initWithGroupName:(NSString *)groupName section:(NSInteger)section
- (instancetype)initWithGroupName:(NSString *)groupName section:(NSInteger)section isCollapsible:(BOOL)isCollapsible
{
if (!(self = [super init]))
return nil;

_section = section;
_isCollapsible = isCollapsible;

auto tapGestureRecognizer = adoptNS([[UITapGestureRecognizer alloc] initWithTarget:self action:@selector(didTapHeader:)]);
[self addGestureRecognizer:tapGestureRecognizer.get()];
Expand All @@ -789,33 +792,43 @@ - (instancetype)initWithGroupName:(NSString *)groupName section:(NSInteger)secti
[_label setAdjustsFontForContentSizeCategory:YES];
[_label setAdjustsFontSizeToFitWidth:NO];
[_label setLineBreakMode:NSLineBreakByTruncatingTail];
[_label setTranslatesAutoresizingMaskIntoConstraints:NO];
[self addSubview:_label.get()];

_collapseIndicatorView = adoptNS([[UIImageView alloc] initWithImage:[UIImage systemImageNamed:@"chevron.down"]]);
[_collapseIndicatorView setPreferredSymbolConfiguration:[UIImageSymbolConfiguration configurationWithFont:WKSelectPickerGroupHeaderView.preferredFont scale:UIImageSymbolScaleSmall]];
[_collapseIndicatorView setContentCompressionResistancePriority:UILayoutPriorityRequired forAxis:UILayoutConstraintAxisHorizontal];
[self addSubview:_collapseIndicatorView.get()];

[_label setTranslatesAutoresizingMaskIntoConstraints:NO];
[NSLayoutConstraint activateConstraints:@[
[[_label leadingAnchor] constraintEqualToAnchor:[self leadingAnchor] constant:WKSelectPickerGroupHeaderView.preferredMargin],
[[_label trailingAnchor] constraintEqualToAnchor:[_collapseIndicatorView leadingAnchor] constant:-groupHeaderLabelImageMargin],
[[_label topAnchor] constraintEqualToAnchor:[self topAnchor] constant:0],
]];

[_collapseIndicatorView setTranslatesAutoresizingMaskIntoConstraints:NO];
[NSLayoutConstraint activateConstraints:@[
[[_collapseIndicatorView trailingAnchor] constraintEqualToAnchor:[self trailingAnchor] constant:-WKSelectPickerGroupHeaderView.preferredMargin],
[[_collapseIndicatorView topAnchor] constraintEqualToAnchor:[_label topAnchor] constant:0],
[[_collapseIndicatorView bottomAnchor] constraintEqualToAnchor:[_label bottomAnchor] constant:0],
]];
if (_isCollapsible) {
_collapseIndicatorView = adoptNS([[UIImageView alloc] initWithImage:[UIImage systemImageNamed:@"chevron.down"]]);
[_label setTranslatesAutoresizingMaskIntoConstraints:NO];
[_collapseIndicatorView setPreferredSymbolConfiguration:[UIImageSymbolConfiguration configurationWithFont:WKSelectPickerGroupHeaderView.preferredFont scale:UIImageSymbolScaleSmall]];
[_collapseIndicatorView setContentCompressionResistancePriority:UILayoutPriorityRequired forAxis:UILayoutConstraintAxisHorizontal];
[_collapseIndicatorView setTranslatesAutoresizingMaskIntoConstraints:NO];
[self addSubview:_collapseIndicatorView.get()];

[NSLayoutConstraint activateConstraints:@[
[[_label leadingAnchor] constraintEqualToAnchor:[self leadingAnchor] constant:WKSelectPickerGroupHeaderView.preferredMargin],
[[_label trailingAnchor] constraintEqualToAnchor:[_collapseIndicatorView leadingAnchor] constant:-groupHeaderLabelImageMargin],
[[_label topAnchor] constraintEqualToAnchor:[self topAnchor] constant:0],
]];

[NSLayoutConstraint activateConstraints:@[
[[_collapseIndicatorView trailingAnchor] constraintEqualToAnchor:[self trailingAnchor] constant:-WKSelectPickerGroupHeaderView.preferredMargin],
[[_collapseIndicatorView topAnchor] constraintEqualToAnchor:[_label topAnchor] constant:0],
[[_collapseIndicatorView bottomAnchor] constraintEqualToAnchor:[_label bottomAnchor] constant:0],
]];
} else {
[NSLayoutConstraint activateConstraints:@[
[[_label leadingAnchor] constraintEqualToAnchor:[self leadingAnchor] constant:WKSelectPickerGroupHeaderView.preferredMargin],
[[_label trailingAnchor] constraintEqualToAnchor:[self trailingAnchor] constant:-WKSelectPickerGroupHeaderView.preferredMargin],
[[_label topAnchor] constraintEqualToAnchor:[self topAnchor]],
[[_label bottomAnchor] constraintEqualToAnchor:[self bottomAnchor]],
]];
}

return self;
}

- (void)setCollapsed:(BOOL)collapsed animated:(BOOL)animated
{
if (_collapsed == collapsed)
if (!_isCollapsible || _collapsed == collapsed)
return;

_collapsed = collapsed;
Expand Down Expand Up @@ -956,7 +969,7 @@ - (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger

- (CGFloat)tableView:(UITableView *)tableView heightForHeaderInSection:(NSInteger)section
{
if (!section)
if (!section || ![[self tableView:tableView titleForHeaderInSection:section] length])
return tableView.layoutMargins.left;

return WKSelectPickerGroupHeaderView.preferredHeight;
Expand Down Expand Up @@ -998,7 +1011,13 @@ - (UIView *)tableView:(UITableView *)tableView viewForHeaderInSection:(NSInteger
if (!section)
return nil;

auto headerView = adoptNS([[WKSelectPickerGroupHeaderView alloc] initWithGroupName:[self tableView:tableView titleForHeaderInSection:section] section:section]);
auto title = [self tableView:tableView titleForHeaderInSection:section];
if (!title.length)
return nil;

BOOL isCollapsible = [self numberOfRowsInGroup:section] > 0;

auto headerView = adoptNS([[WKSelectPickerGroupHeaderView alloc] initWithGroupName:title section:section isCollapsible:isCollapsible]);
[headerView setCollapsed:[_collapsedSections containsObject:@(section)] animated:NO];
[headerView setTableViewController:self];

Expand All @@ -1007,6 +1026,9 @@ - (UIView *)tableView:(UITableView *)tableView viewForHeaderInSection:(NSInteger

- (void)didTapSelectPickerGroupHeaderView:(WKSelectPickerGroupHeaderView *)headerView
{
if (!headerView.isCollapsible)
return;

NSInteger section = headerView.section;
NSInteger rowCount = [self numberOfRowsInGroup:section];

Expand Down
25 changes: 19 additions & 6 deletions Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
Original file line number Diff line number Diff line change
Expand Up @@ -3607,19 +3607,32 @@ static void handleAnimationActions(Element& element, uint32_t action)
information.ariaLabel = focusedElement->attributeWithoutSynchronization(HTMLNames::aria_labelAttr);

if (is<HTMLSelectElement>(*focusedElement)) {
#if USE(UICONTEXTMENU)
static bool selectPickerUsesMenu = linkedOnOrAfterSDKWithBehavior(SDKAlignedBehavior::HasUIContextMenuInteraction);
#else
bool selectPickerUsesMenu = false;
#endif

HTMLSelectElement& element = downcast<HTMLSelectElement>(*focusedElement);
information.elementType = InputType::Select;

RefPtr<ContainerNode> parentGroup;
int parentGroupID = 0;
// The parent group ID indicates the group the option belongs to and is 0 for group elements.
// If there are option elements in between groups, they are given it's own group identifier.
// If a select does not have groups, all the option elements have group ID 0.
for (auto& item : element.listItems()) {
if (auto* optionElement = dynamicDowncast<HTMLOptionElement>(item.get()))
if (auto* optionElement = dynamicDowncast<HTMLOptionElement>(item.get())) {
if (parentGroup && optionElement->parentNode() != parentGroup) {
parentGroupID++;
parentGroup = nullptr;
information.selectOptions.append(OptionItem(emptyString(), true, false, false, parentGroupID));
}

information.selectOptions.append(OptionItem(optionElement->displayLabel(), false, optionElement->selected(), optionElement->hasAttributeWithoutSynchronization(WebCore::HTMLNames::disabledAttr), parentGroupID));
else if (auto* optGroupElement = dynamicDowncast<HTMLOptGroupElement>(item.get())) {
} else if (auto* optGroupElement = dynamicDowncast<HTMLOptGroupElement>(item.get())) {
if (selectPickerUsesMenu)
parentGroup = optGroupElement;

parentGroupID++;
information.selectOptions.append(OptionItem(optGroupElement->groupLabelText(), true, false, optGroupElement->hasAttributeWithoutSynchronization(WebCore::HTMLNames::disabledAttr), 0));
information.selectOptions.append(OptionItem(optGroupElement->groupLabelText(), true, false, optGroupElement->hasAttributeWithoutSynchronization(WebCore::HTMLNames::disabledAttr), parentGroupID));
}
}
information.selectedIndex = element.selectedIndex();
Expand Down

0 comments on commit 4d943d6

Please sign in to comment.