[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 #22 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

>> 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;

Or no helper functions and just start with this preamble:

    {
        HTMLSelectElement* selectElement;
        bool isGroup;
        if (is<HTMLOptionElement>(element)) {
            selectElement = downcast<HTMLOptionElement>(element).ownerSelectElement();
            isGroup = false;
        } else if (is<HTMLOptGroupElement>(element)) {
            selectElement = downcast<HTMLOptGroupElement>(element).ownerSelectElement();
            isGroup = true;
        } else
            return std::nullopt;
        auto& downcastedElement = downcast<HTMLElement>(element);

>> Source/WebCore/dom/Element.cpp:1230
>> +            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) });
>         }
>     ...

Sorry, I meant:

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

No need to call isOptionOrGroup here.

-- 
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/90a5c3cc/attachment.html>