[Webkit-unassigned] [Bug 175016] getClientRects doesn't work with list box option elements

[bugzilla-daemon at webkit.org]

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

--- Comment #21 from Darin Adler <darin at apple.com> ---
Comment on attachment 316950
  --> https://bugs.webkit.org/attachment.cgi?id=316950
Updated patch

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

This seems like an improvement over the earlier version.

Not a huge fan of the use of variant just to deal with the two different ways of getting the select element. I would consider something much simpler using overloading or something.

I would have made these two helper functions:

    bool isOptionOrGroup(Element&);
    HTMLSelectElement* selectForOptionOrGroup(Element&);

> Source/WebCore/dom/Element.cpp:1152
> +static std::optional<LayoutRect> listBoxElementBoundingBox(Variant<RefPtr<HTMLOptionElement>, RefPtr<HTMLOptGroupElement>> optionOrOptGroupElement, RenderObject*& renderer)

Seems strange to use RefPtr in the variant. Why not just a plain old pointer? Also ugly to have the out argument. If we need to return both a renderer and a rectangle, then they should both be return values:

    static std::optional<std::pair<RenderObject*, LayoutRect>> listBoxElementBoundingBox(Element& element)
    {
        if (!isOptionOrGroup(element))
            return std::nullopt;

> Source/WebCore/dom/Element.cpp:1156
> +    auto* selectElement = WTF::switchOn(optionOrOptGroupElement,
> +        [](const auto& element) -> HTMLSelectElement* { return element->ownerSelectElement(); }
> +    );

auto* selectElement = selectForOptionOrGroup(element);

> Source/WebCore/dom/Element.cpp:1162
> +    renderer = selectElement->renderer();
> +    auto& listBoxRenderer = downcast<RenderListBox>(*renderer);

merge into a single line, and just name it renderer, I think

> Source/WebCore/dom/Element.cpp:1168
> +    size_t optionIndex = 0;

The type of the argument to itemBoundingBoxRect is "int", not "size_t".

> Source/WebCore/dom/Element.cpp:1173
> +            if (is<HTMLOptionElement>(element))

Strange to do this test again inside the loop like this after going to all the trouble of using a variant.

> Source/WebCore/dom/Element.cpp:1174
> +                return boundingBox;

break;

> Source/WebCore/dom/Element.cpp:1175
> +        } else if (is<HTMLOptGroupElement>(element) && boundingBox) {

Strange to do this test again inside the loop like this after going to all the trouble of using a variant.

> Source/WebCore/dom/Element.cpp:1177
> +                return boundingBox;

break;

> Source/WebCore/dom/Element.cpp:1184
> +    return boundingBox;

if (!boundingBox)
        return std::nullopt;
    return { &renderer, boundingBox.value() }; // <<< with whatever explicit type names are needed to make it compile

> Source/WebCore/dom/Element.cpp:1230
> +    } else if (is<HTMLOptionElement>(*this)) {
> +        if (auto boundingBox = listBoxElementBoundingBox(downcast<HTMLOptionElement>(this), renderer))
> +            quads.append(renderer->localToAbsoluteQuad(FloatQuad(boundingBox.value())));
> +    } else if (is<HTMLOptGroupElement>(*this)) {
> +        if (auto boundingBox = listBoxElementBoundingBox(downcast<HTMLOptGroupElement>(this), renderer))
> +            quads.append(renderer->localToAbsoluteQuad(FloatQuad(boundingBox.value())));

And I would write something more like this:

    } else if (isOptionOrGroup(*this)) {
        if (auto pair = listBoxElementBoundingBox(*this)) {
            renderer = pair.value().first;
            quads.append(renderer->localToAbsoluteQuad(FloatQuad { pair.value().second) });
        }
    ...

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170804/e507584d/attachment-0001.html>