[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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com

--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 316844
  --> https://bugs.webkit.org/attachment.cgi?id=316844
Patch

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

> Source/WebCore/dom/Element.cpp:1154
> +    if (is<HTMLOptionElement>(*this) || is<HTMLOptGroupElement>(*this)) {

This kind of thing is really unfortunate for long term maintainability of our code. But we do have a lot of code like this. We could consider a more traditional solution where we use virtual functions instead of sprinkling type-specific checks into base classes.

> Source/WebCore/dom/Element.cpp:1158
> +            Vector<FloatQuad> quads = { rect };
> +            return DOMRectList::create(quads);

Reads nicely on a single line:

    return DOMRectList::create(Vector<FloatQuad> { rect });

> Source/WebCore/dom/Element.cpp:1211
> +        // Get the bounding box of the select items individually in case of list box.
> +        auto* selectElement = is<HTMLOptionElement>(*this) ? downcast<HTMLOptionElement>(*this).ownerSelectElement() : downcast<HTMLOptGroupElement>(*this).ownerSelectElement();
> +        if (selectElement && selectElement->renderer() && is<RenderListBox>(selectElement->renderer())) {
> +            renderer = selectElement->renderer();
> +            auto& listBoxRenderer = downcast<RenderListBox>(*renderer);
> +            LayoutRect boundingBox;
> +            size_t optionIndex = 0;
> +            for (auto* item : selectElement->listItems()) {
> +                if (item == this) {
> +                    LayoutPoint additionOffset;
> +                    boundingBox = listBoxRenderer.itemBoundingBoxRect(additionOffset, optionIndex);
> +                    if (!is<HTMLOptGroupElement>(*this))
> +                        break;
> +                } else if (is<HTMLOptGroupElement>(*this) && !boundingBox.isEmpty()) {
> +                    if (item->parentNode() != this)
> +                        break;
> +                    LayoutPoint additionOffset;
> +                    boundingBox.setHeight(boundingBox.height() + listBoxRenderer.itemBoundingBoxRect(additionOffset, optionIndex).height());
> +                }
> +                ++optionIndex;
> +            }
> +            quads.append(renderer->localToAbsoluteQuad(FloatQuad(boundingBox)));

This is big enough that I think we should consider putting it in a separate function. It’s bigger than the entire rest of the function combined and overwhelms the main logic 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/20170801/3f621df2/attachment-0001.html>