Skip to content

Commit

Permalink
Bug 1300405 - Fix insertion of already selected options r=emilio
Browse files Browse the repository at this point in the history 
Differential Revision: https://phabricator.services.mozilla.com/D166955
  • Loading branch information
@ben-freist
ben-freist committed Feb 12, 2023
1 parent 40ed51e commit 3abd2c7
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 59 deletions.
13 changes: 8 additions & 5 deletions dom/html/HTMLOptionElement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,11 @@ void HTMLOptionElement::SetSelected(bool aValue) {
HTMLSelectElement* selectInt = GetSelect();
if (selectInt) {
int32_t index = Index();
uint32_t mask = HTMLSelectElement::SET_DISABLED | HTMLSelectElement::NOTIFY;
HTMLSelectElement::OptionFlags mask{
HTMLSelectElement::OptionFlag::SetDisabled,
HTMLSelectElement::OptionFlag::Notify};
if (aValue) {
mask |= HTMLSelectElement::IS_SELECTED;
mask += HTMLSelectElement::OptionFlag::IsSelected;
}

// This should end up calling SetSelectedInternal
Expand Down Expand Up @@ -169,13 +171,14 @@ nsresult HTMLOptionElement::BeforeSetAttr(int32_t aNamespaceID, nsAtom* aName,
mIsInSetDefaultSelected = true;

int32_t index = Index();
uint32_t mask = HTMLSelectElement::SET_DISABLED;
HTMLSelectElement::OptionFlags mask =
HTMLSelectElement::OptionFlag::SetDisabled;
if (aValue) {
mask |= HTMLSelectElement::IS_SELECTED;
mask += HTMLSelectElement::OptionFlag::IsSelected;
}

if (aNotify) {
mask |= HTMLSelectElement::NOTIFY;
mask += HTMLSelectElement::OptionFlag::Notify;
}

// This can end up calling SetSelectedInternal if our selected state needs to
Expand Down
68 changes: 42 additions & 26 deletions dom/html/HTMLSelectElement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,9 @@ void HTMLSelectElement::InsertOptionsIntoList(nsIContent* aOptions,
if (option && option->Selected()) {
// Clear all other options
if (!HasAttr(kNameSpaceID_None, nsGkAtoms::multiple)) {
uint32_t mask = IS_SELECTED | CLEAR_ALL | SET_DISABLED | NOTIFY;
OptionFlags mask{OptionFlag::IsSelected, OptionFlag::ClearAll,
OptionFlag::SetDisabled, OptionFlag::Notify,
OptionFlag::InsertingOptions};
SetOptionsSelectedByIndex(i, i, mask);
}

Expand Down Expand Up @@ -632,9 +634,10 @@ nsIHTMLCollection* HTMLSelectElement::SelectedOptions() {

void HTMLSelectElement::SetSelectedIndexInternal(int32_t aIndex, bool aNotify) {
int32_t oldSelectedIndex = mSelectedIndex;
uint32_t mask = IS_SELECTED | CLEAR_ALL | SET_DISABLED;
OptionFlags mask{OptionFlag::IsSelected, OptionFlag::ClearAll,
OptionFlag::SetDisabled};
if (aNotify) {
mask |= NOTIFY;
mask += OptionFlag::Notify;
}

SetOptionsSelectedByIndex(aIndex, aIndex, mask);
Expand Down Expand Up @@ -721,14 +724,14 @@ void HTMLSelectElement::FindSelectedIndex(int32_t aStartIndex, bool aNotify) {
// setDisabled vs checkDisabled business.
bool HTMLSelectElement::SetOptionsSelectedByIndex(int32_t aStartIndex,
int32_t aEndIndex,
uint32_t aOptionsMask) {
OptionFlags aOptionsMask) {
#if 0
printf("SetOption(%d-%d, %c, ClearAll=%c)\n", aStartIndex, aEndIndex,
(aOptionsMask & IS_SELECTED ? 'Y' : 'N'),
(aOptionsMask & CLEAR_ALL ? 'Y' : 'N'));
(aOptionsMask.contains(OptionFlag::IsSelected) ? 'Y' : 'N'),
(aOptionsMask.contains(OptionFlag::ClearAll) ? 'Y' : 'N'));
#endif
// Don't bother if the select is disabled
if (!(aOptionsMask & SET_DISABLED) && IsDisabled()) {
if (!aOptionsMask.contains(OptionFlag::SetDisabled) && IsDisabled()) {
return false;
}

Expand All @@ -750,7 +753,7 @@ bool HTMLSelectElement::SetOptionsSelectedByIndex(int32_t aStartIndex,
bool didGetFrame = false;
AutoWeakFrame weakSelectFrame;

if (aOptionsMask & IS_SELECTED) {
if (aOptionsMask.contains(OptionFlag::IsSelected)) {
// Setting selectedIndex to an out-of-bounds index means -1. (HTML5)
if (aStartIndex < 0 || AssertedCast<uint32_t>(aStartIndex) >= numItems ||
aEndIndex < 0 || AssertedCast<uint32_t>(aEndIndex) >= numItems) {
Expand All @@ -767,7 +770,7 @@ bool HTMLSelectElement::SetOptionsSelectedByIndex(int32_t aStartIndex,
// select are disabled. If ClearAll is passed in as true, and we do not
// select anything because the options are disabled, we will not clear the
// other options. (This is to make the UI work the way one might expect.)
bool allDisabled = !(aOptionsMask & SET_DISABLED);
bool allDisabled = !aOptionsMask.contains(OptionFlag::SetDisabled);

//
// Save a little time when clearing other options
Expand All @@ -788,15 +791,17 @@ bool HTMLSelectElement::SetOptionsSelectedByIndex(int32_t aStartIndex,
RefPtr<HTMLOptionElement> option = Item(optIndex);

// Ignore disabled options.
if (!(aOptionsMask & SET_DISABLED)) {
if (!aOptionsMask.contains(OptionFlag::SetDisabled)) {
if (option && IsOptionDisabled(option)) {
continue;
}
allDisabled = false;
}

// If the index is already selected, ignore it.
if (option && !option->Selected()) {
// If the index is already selected, ignore it. On the other hand when
// the option has just been inserted we have to get in sync with it.
if (option && (aOptionsMask.contains(OptionFlag::InsertingOptions) ||
!option->Selected())) {
// To notify the frame if anything gets changed. No need
// to flush here, if there's no frame yet we don't need to
// force it to be created just to notify it about a change
Expand All @@ -805,8 +810,8 @@ bool HTMLSelectElement::SetOptionsSelectedByIndex(int32_t aStartIndex,
weakSelectFrame = do_QueryFrame(selectFrame);
didGetFrame = true;

OnOptionSelected(selectFrame, optIndex, true, true,
aOptionsMask & NOTIFY);
OnOptionSelected(selectFrame, optIndex, true, !option->Selected(),
aOptionsMask.contains(OptionFlag::Notify));
optionsSelected = true;
}
}
Expand All @@ -815,14 +820,15 @@ bool HTMLSelectElement::SetOptionsSelectedByIndex(int32_t aStartIndex,
// Next remove all other options if single select or all is clear
// If index is -1, everything will be deselected (bug 28143)
if (((!isMultiple && optionsSelected) ||
((aOptionsMask & CLEAR_ALL) && !allDisabled) || aStartIndex == -1) &&
(aOptionsMask.contains(OptionFlag::ClearAll) && !allDisabled) ||
aStartIndex == -1) &&
previousSelectedIndex != -1) {
for (uint32_t optIndex = AssertedCast<uint32_t>(previousSelectedIndex);
optIndex < numItems; optIndex++) {
if (static_cast<int32_t>(optIndex) < aStartIndex ||
static_cast<int32_t>(optIndex) > aEndIndex) {
HTMLOptionElement* option = Item(optIndex);
// If the index is already selected, ignore it.
// If the index is already deselected, ignore it.
if (option && option->Selected()) {
if (!didGetFrame || (selectFrame && !weakSelectFrame.IsAlive())) {
// To notify the frame if anything gets changed, don't
Expand All @@ -835,7 +841,7 @@ bool HTMLSelectElement::SetOptionsSelectedByIndex(int32_t aStartIndex,
}

OnOptionSelected(selectFrame, optIndex, false, true,
aOptionsMask & NOTIFY);
aOptionsMask.contains(OptionFlag::Notify));
optionsDeselected = true;

// Only need to deselect one option if not multiple
Expand All @@ -851,7 +857,8 @@ bool HTMLSelectElement::SetOptionsSelectedByIndex(int32_t aStartIndex,
// any that are in the specified range.
for (int32_t optIndex = aStartIndex; optIndex <= aEndIndex; optIndex++) {
HTMLOptionElement* option = Item(optIndex);
if (!(aOptionsMask & SET_DISABLED) && IsOptionDisabled(option)) {
if (!aOptionsMask.contains(OptionFlag::SetDisabled) &&
IsOptionDisabled(option)) {
continue;
}

Expand All @@ -868,16 +875,18 @@ bool HTMLSelectElement::SetOptionsSelectedByIndex(int32_t aStartIndex,
}

OnOptionSelected(selectFrame, optIndex, false, true,
aOptionsMask & NOTIFY);
aOptionsMask.contains(OptionFlag::Notify));
optionsDeselected = true;
}
}
}

// Make sure something is selected unless we were set to -1 (none)
if (optionsDeselected && aStartIndex != -1 && !(aOptionsMask & NO_RESELECT)) {
if (optionsDeselected && aStartIndex != -1 &&
!aOptionsMask.contains(OptionFlag::NoReselect)) {
optionsSelected =
CheckSelectSomething(aOptionsMask & NOTIFY) || optionsSelected;
CheckSelectSomething(aOptionsMask.contains(OptionFlag::Notify)) ||
optionsSelected;
}

// Let the caller know whether anything was changed
Expand Down Expand Up @@ -1321,15 +1330,18 @@ void HTMLSelectElement::RestoreStateTo(const SelectContentData& aNewSelected) {
}

uint32_t len = Length();
uint32_t mask = IS_SELECTED | CLEAR_ALL | SET_DISABLED | NOTIFY;
OptionFlags mask{OptionFlag::IsSelected, OptionFlag::ClearAll,
OptionFlag::SetDisabled, OptionFlag::Notify};

// First clear all
SetOptionsSelectedByIndex(-1, -1, mask);

// Select by index.
for (uint32_t idx : aNewSelected.indices()) {
if (idx < len) {
SetOptionsSelectedByIndex(idx, idx, IS_SELECTED | SET_DISABLED | NOTIFY);
SetOptionsSelectedByIndex(idx, idx,
{OptionFlag::IsSelected,
OptionFlag::SetDisabled, OptionFlag::Notify});
}
}

Expand All @@ -1340,7 +1352,10 @@ void HTMLSelectElement::RestoreStateTo(const SelectContentData& aNewSelected) {
nsAutoString value;
option->GetValue(value);
if (aNewSelected.values().Contains(value)) {
SetOptionsSelectedByIndex(i, i, IS_SELECTED | SET_DISABLED | NOTIFY);
SetOptionsSelectedByIndex(
i, i,
{OptionFlag::IsSelected, OptionFlag::SetDisabled,
OptionFlag::Notify});
}
}
}
Expand All @@ -1364,9 +1379,10 @@ HTMLSelectElement::Reset() {
// Reset the option to its default value
//

uint32_t mask = SET_DISABLED | NOTIFY | NO_RESELECT;
OptionFlags mask = {OptionFlag::SetDisabled, OptionFlag::Notify,
OptionFlag::NoReselect};
if (option->DefaultSelected()) {
mask |= IS_SELECTED;
mask += OptionFlag::IsSelected;
numSelected++;
}

Expand Down
38 changes: 22 additions & 16 deletions dom/html/HTMLSelectElement.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "mozilla/dom/BindingDeclarations.h"
#include "mozilla/dom/UnionTypes.h"
#include "mozilla/dom/HTMLOptionsCollection.h"
#include "mozilla/EnumSet.h"
#include "nsCheapSets.h"
#include "nsCOMPtr.h"
#include "nsError.h"
Expand Down Expand Up @@ -77,26 +78,31 @@ class HTMLSelectElement final : public nsGenericHTMLFormControlElementWithState,
public ConstraintValidation {
public:
/**
* IS_SELECTED whether to set the option(s) to true or false
* IsSelected whether to set the option(s) to true or false
*
* CLEAR_ALL whether to clear all other options (for example, if you
* are normal-clicking on the current option)
* ClearAll whether to clear all other options (for example, if you
* are normal-clicking on the current option)
*
* SET_DISABLED whether it is permissible to set disabled options
* (for JavaScript)
* SetDisabled whether it is permissible to set disabled options
* (for JavaScript)
*
* NOTIFY whether to notify frames and such
* Notify whether to notify frames and such
*
* NO_RESELECT no need to select something after an option is deselected
* (for reset)
*/
enum OptionType {
IS_SELECTED = 1 << 0,
CLEAR_ALL = 1 << 1,
SET_DISABLED = 1 << 2,
NOTIFY = 1 << 3,
NO_RESELECT = 1 << 4
* NoReselect no need to select something after an option is
* deselected (for reset)
*
* InsertingOptions if an option has just been inserted some bailouts can't
* be taken
*/
enum class OptionFlag : uint8_t {
IsSelected,
ClearAll,
SetDisabled,
Notify,
NoReselect,
InsertingOptions
};
using OptionFlags = EnumSet<OptionFlag>;

using ConstraintValidation::GetValidationMessage;

Expand Down Expand Up @@ -258,7 +264,7 @@ class HTMLSelectElement final : public nsGenericHTMLFormControlElementWithState,
* @return whether any options were actually changed
*/
bool SetOptionsSelectedByIndex(int32_t aStartIndex, int32_t aEndIndex,
uint32_t aOptionsMask);
OptionFlags aOptionsMask);

/**
* Called when an attribute is about to be changed
Expand Down
24 changes: 12 additions & 12 deletions layout/forms/nsListControlFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -887,34 +887,34 @@ bool nsListControlFrame::SetOptionsSelectedFromFrame(int32_t aStartIndex,
int32_t aEndIndex,
bool aValue,
bool aClearAll) {
RefPtr<dom::HTMLSelectElement> selectElement =
dom::HTMLSelectElement::FromNode(mContent);
using OptionFlag = HTMLSelectElement::OptionFlag;
RefPtr<HTMLSelectElement> selectElement =
HTMLSelectElement::FromNode(mContent);

uint32_t mask = dom::HTMLSelectElement::NOTIFY;
HTMLSelectElement::OptionFlags mask = OptionFlag::Notify;
if (mForceSelection) {
mask |= dom::HTMLSelectElement::SET_DISABLED;
mask += OptionFlag::SetDisabled;
}
if (aValue) {
mask |= dom::HTMLSelectElement::IS_SELECTED;
mask += OptionFlag::IsSelected;
}
if (aClearAll) {
mask |= dom::HTMLSelectElement::CLEAR_ALL;
mask += OptionFlag::ClearAll;
}

return selectElement->SetOptionsSelectedByIndex(aStartIndex, aEndIndex, mask);
}

bool nsListControlFrame::ToggleOptionSelectedFromFrame(int32_t aIndex) {
RefPtr<dom::HTMLOptionElement> option =
GetOption(static_cast<uint32_t>(aIndex));
RefPtr<HTMLOptionElement> option = GetOption(static_cast<uint32_t>(aIndex));
NS_ENSURE_TRUE(option, false);

RefPtr<dom::HTMLSelectElement> selectElement =
dom::HTMLSelectElement::FromNode(mContent);
RefPtr<HTMLSelectElement> selectElement =
HTMLSelectElement::FromNode(mContent);

uint32_t mask = dom::HTMLSelectElement::NOTIFY;
HTMLSelectElement::OptionFlags mask = HTMLSelectElement::OptionFlag::Notify;
if (!option->Selected()) {
mask |= dom::HTMLSelectElement::IS_SELECTED;
mask += HTMLSelectElement::OptionFlag::IsSelected;
}

return selectElement->SetOptionsSelectedByIndex(aIndex, aIndex, mask);
Expand Down
16 changes: 16 additions & 0 deletions testing/web-platform/tests/html/semantics/forms/the-select-element/inserted-or-removed.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,5 +83,21 @@
'<option selected>Fourth</option>';
assert_equals(target.selectedOptions[0].textContent, 'Fourth');
}, 'The last selected OPTION should win; Inserted by innerHTML');

test (() => {
for (let insert_location = 0; insert_location < 3; ++insert_location) {
const target = document.querySelector('#by-innerHTML');
target.innerHTML = '<option>A</option>' +
'<option selected>C</option>' +
'<option>D</option>';
const refNode = target.querySelectorAll('option')[insert_location];

const opt = document.createElement('option');
opt.selected = true;
opt.textContent = 'B';
target.insertBefore(opt, refNode);
assert_equals(target.selectedOptions[0].textContent, 'B');
}
}, 'If an OPTION says it is selected, it should be selected after it is inserted.');
</script>
</body>

0 comments on commit 3abd2c7

Please sign in to comment.