Bug 155696 – AX: Consolidate radio button group member code with that in HTMLElement derivatives

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[chris fleizach]

From

https://bugs.webkit.org/show_bug.cgi?id=155604

> The old code is not the right way to find radio buttons. Code to iterate all
> the radio buttons in a group should not have been replicated in the
> accessibility code, but instead should be an interface provide by the
> HTMLInputElement class. We could have a function:
> 
>      Vector<HTMLInputElement*> radioButtonGroup() const;
> 
> It would get the misnamed CheckedRadioButtons object, then would take the
> element's name and find the RadioButtonGroup. That has a HashSet of all the
> radio buttons in that group and that can be converted into a Vector.

[Radar WebKit Bug Importer]

<rdar://problem/25260379>

[chris fleizach]

Created attachment 274633 [details]
patch

[WebKit Commit Bot]

Attachment 274633 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/CheckedRadioButtons.cpp:204:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 274634 [details]
patch

[Darin Adler]

Comment on attachment 274634 [details]
patch

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

Thanks so much for tackling this!

There’s a serious problem here with the missing empty name check in CheckedRadioButtons::groupMembers.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:970
> +        for (auto radioSibling : input.radioButtonGroup()) {

It’s a little inaccurate to call this a “sibling” since that term is normally reserved for elements that have the same parent. The radio buttons in a radio button group are all members of the group but not necessarily siblings in the DOM tree sense.

> Source/WebCore/dom/CheckedRadioButtons.cpp:69
> +Vector<HTMLInputElement*> RadioButtonGroup::members() const
> +{
> +    Vector<HTMLInputElement*> members;
> +    copyToVector(m_members, members);
> +    return members;
> +}

Newfangled idea, not critical: Since none of the radio buttons in the vector can be null, we might want to be super-modern and make the type of the elements be std::reference_wrapper<HTMLInputElement> instead of HTMLInputElement* to express the fact that they can never be null. It’s easy to make one using std::ref. This is not something we‘ve done in existing code in WebKit in the past, but I probably a good way to go for future code. Unfortunately that would mean we couldn’t use copyToVector in its current form. On the other hand, if we did write it out we could actually optimize it better because we could use reserveInitialCapacity and uncheckedAppend, which is more efficient than what copyToVector currently does with resize and assignment. Long term we should also make a better replacement for copyToVector that creates a new vector rather than overwriting an existing vector.

> Source/WebCore/dom/CheckedRadioButtons.cpp:207
> +    if (!element || !element->isRadioButton())
> +        return Vector<HTMLInputElement*>();
> +
> +    if (RadioButtonGroup* group = m_nameToGroupMap->get(element->name().impl()))
> +        return group->members();
> +    return Vector<HTMLInputElement*>();

This function is missing the “name empty” check that other functions in this class have. We need some kind of check because the hash map does not support null keys: if the name is null, I believe the “get” function will crash.

We typically prefer early return and use the { } syntax for empty results rather than naming the type:

    if (!element.isRadioButton())
        return { };
    auto* name = element.name().impl();
    if (!name)
        return name;
    auto* group = m_nameToGroupMap->get(name);
    if (!group)
        return { };
    return group->members();

All the other functions in this class assert that the passed in element is a radio button. Not sure why we chose a different approach for this function.

> Source/WebCore/dom/CheckedRadioButtons.h:46
> +    Vector<HTMLInputElement*> groupMembers(HTMLInputElement*) const;

Argument should be HTMLInputElement&, not HTMLInputElement*. This differs from the older functions, but eventually those should be fixed too.

> Source/WebCore/html/HTMLInputElement.cpp:1782
> +static inline bool domNodeSort(HTMLInputElement* lhs, HTMLInputElement* rhs)
> +{
> +    return Range::compareBoundaryPoints(lhs, 0, rhs, 0, IGNORE_EXCEPTION) < 0;
> +}

I don’t think a comparison function in the “less than” comparator predicate style should be called “sort”. I would write this:

    static inline bool documentOrderLessThan(const Node* a, const Node* b)
    {
        return Range::compareBoundaryPoints(lhs, 0, rhs, 0, ASSERT_NO_EXCEPTION) < 0;
    }

Or we could call it documentOrderComparator or something like that. I could also imagine putting this into a header to make it reusable. I’m surprised that this is the first place we’ve had to sort nodes by document order!

Given that the only exception is “not in the same document”, which I believe cannot happen, I think ASSERT_NOT_EXCEPTION is what we want, rather than IGNORE_EXCEPTION.

> Source/WebCore/html/HTMLInputElement.cpp:1792
> +    ASSERT(isRadioButton());
> +    if (CheckedRadioButtons* buttons = checkedRadioButtons()) {
> +        Vector<HTMLInputElement*> group = buttons->groupMembers(this);
> +        std::stable_sort(group.begin(), group.end(), domNodeSort);
> +        return group;
> +    }
> +    return Vector<HTMLInputElement*>();

I would prefer we return an empty vector rather than doing the assertion if the input element is not a radio button. So we should just remove that assertion. That’s the same way other functions in this class handle it.

Would be more normal WebKit style to use early exit instead of nesting the normal case code in an if statement.

As far as I can tell, there is no need for stability in the sort; no two input elements should compare as equal. Thus I would suggest using std::sort instead of std::stable_sort, unless there’s some problem with that.

I think it’s a bit peculiar to make the lower levels convert from a set to a vector but then do sorting here at the highest level in HTMLInputElement::radioButtonGroup. There’s nothing about the lower levels that makes the sorting less appropriate there and more appropriate here. I could imagine these two other options that both seem slightly better to me:

1) Putting the sorting into RadioButtonGroup::members; would be logical to do the sorting as soon as we turn the set into a vector.

2) Having all these functions return a const HashSet<HTMLInputElement*>& and then do the sorting in the accessibility code. We can have one or more global empty sets that we use for all the cases where we need to return the empty set.

One of the advantages of (1) is that some day we might decide to keep the radio button groups sorted by document order. If we do that, we’d be doing it in RadioButtonGroup and it would be nice not to have to change any of the higher level functions.

> Source/WebCore/html/HTMLInputElement.h:274
> +    Vector<HTMLInputElement*> radioButtonGroup();

Should be const, like checkedRadioButtonForGroup below. Not sure why isInRequiredRadioButtonGroup is not marked const.

[chris fleizach]

Comment on attachment 274634 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:970
>> +        for (auto radioSibling : input.radioButtonGroup()) {
> 
> It’s a little inaccurate to call this a “sibling” since that term is normally reserved for elements that have the same parent. The radio buttons in a radio button group are all members of the group but not necessarily siblings in the DOM tree sense.

I'll go with radioButtonGroupMember

>> Source/WebCore/dom/CheckedRadioButtons.cpp:69
>> +}
> 
> Newfangled idea, not critical: Since none of the radio buttons in the vector can be null, we might want to be super-modern and make the type of the elements be std::reference_wrapper<HTMLInputElement> instead of HTMLInputElement* to express the fact that they can never be null. It’s easy to make one using std::ref. This is not something we‘ve done in existing code in WebKit in the past, but I probably a good way to go for future code. Unfortunately that would mean we couldn’t use copyToVector in its current form. On the other hand, if we did write it out we could actually optimize it better because we could use reserveInitialCapacity and uncheckedAppend, which is more efficient than what copyToVector currently does with resize and assignment. Long term we should also make a better replacement for copyToVector that creates a new vector rather than overwriting an existing vector.

will give it a shot and see what you think

>> Source/WebCore/dom/CheckedRadioButtons.cpp:207
>> +    return Vector<HTMLInputElement*>();
> 
> This function is missing the “name empty” check that other functions in this class have. We need some kind of check because the hash map does not support null keys: if the name is null, I believe the “get” function will crash.
> 
> We typically prefer early return and use the { } syntax for empty results rather than naming the type:
> 
>     if (!element.isRadioButton())
>         return { };
>     auto* name = element.name().impl();
>     if (!name)
>         return name;
>     auto* group = m_nameToGroupMap->get(name);
>     if (!group)
>         return { };
>     return group->members();
> 
> All the other functions in this class assert that the passed in element is a radio button. Not sure why we chose a different approach for this function.

will fix up

>> Source/WebCore/html/HTMLInputElement.cpp:1782
>> +}
> 
> I don’t think a comparison function in the “less than” comparator predicate style should be called “sort”. I would write this:
> 
>     static inline bool documentOrderLessThan(const Node* a, const Node* b)
>     {
>         return Range::compareBoundaryPoints(lhs, 0, rhs, 0, ASSERT_NO_EXCEPTION) < 0;
>     }
> 
> Or we could call it documentOrderComparator or something like that. I could also imagine putting this into a header to make it reusable. I’m surprised that this is the first place we’ve had to sort nodes by document order!
> 
> Given that the only exception is “not in the same document”, which I believe cannot happen, I think ASSERT_NOT_EXCEPTION is what we want, rather than IGNORE_EXCEPTION.

I was also surprised I couldn't find any other examples of DOM sorting.

I could put this in Range.h, since it leverages compareBoundaryPoints

>> Source/WebCore/html/HTMLInputElement.cpp:1792
>> +    return Vector<HTMLInputElement*>();
> 
> I would prefer we return an empty vector rather than doing the assertion if the input element is not a radio button. So we should just remove that assertion. That’s the same way other functions in this class handle it.
> 
> Would be more normal WebKit style to use early exit instead of nesting the normal case code in an if statement.
> 
> As far as I can tell, there is no need for stability in the sort; no two input elements should compare as equal. Thus I would suggest using std::sort instead of std::stable_sort, unless there’s some problem with that.
> 
> I think it’s a bit peculiar to make the lower levels convert from a set to a vector but then do sorting here at the highest level in HTMLInputElement::radioButtonGroup. There’s nothing about the lower levels that makes the sorting less appropriate there and more appropriate here. I could imagine these two other options that both seem slightly better to me:
> 
> 1) Putting the sorting into RadioButtonGroup::members; would be logical to do the sorting as soon as we turn the set into a vector.
> 
> 2) Having all these functions return a const HashSet<HTMLInputElement*>& and then do the sorting in the accessibility code. We can have one or more global empty sets that we use for all the cases where we need to return the empty set.
> 
> One of the advantages of (1) is that some day we might decide to keep the radio button groups sorted by document order. If we do that, we’d be doing it in RadioButtonGroup and it would be nice not to have to change any of the higher level functions.

Option 1 also sounds good to me

[chris fleizach]

Created attachment 275116 [details]
patch

Address Darin's feedback

I tried playing with the std::reference_wrapper for awhile, but couldn't get it to work out and compile with the existing HashSet<HTMLInputElement*> list that I was converting between. My C++ foo was probably not strong enough

[WebKit Commit Bot]

Attachment 275116 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Range.h:185:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/dom/CheckedRadioButtons.h:29:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 275117 [details]
patch

[Darin Adler]

Comment on attachment 275117 [details]
patch

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

> Source/WebCore/dom/CheckedRadioButtons.h:46
> +    Vector<HTMLInputElement*> groupMembers(HTMLInputElement*) const;

Since the only caller of this function passes "this", the function should take HTMLInputElement&.

[chris fleizach]

http://trac.webkit.org/changeset/198997