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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 1 09:23:26 PDT 2017


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

--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Darin Adler from comment #4)
> Comment on attachment 316844 [details]
> 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.

hmm, maybe we can add a virtual absoluteQuads() to Node? or Element? The default implementation would simply use RenderBoxModelObject::absoluteQuads() and elements not having a renderer would implement it too.

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

Indeed.

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

Ok.

-- 
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/7088bca5/attachment.html>


More information about the webkit-unassigned mailing list