Bug 175016 – getClientRects doesn't work with list box option elements

• Component: DOM
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Since HTMLOptionElement and HTMLOptGroupElement don't have a renderer, we are always returning an empty list from getClientRects. This is working fine in both chromium and firefox, option elements return its own bounding box and group elements return the bounding box of the group label and all its children items.

[Carlos Garcia Campos]

Created attachment 316844 [details]
Patch

[Build Bot]

Comment on attachment 316844 [details]
Patch

Attachment 316844 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4233497

New failing tests:
fast/dom/HTMLSelectElement/listbox-items-client-rects.html

[Build Bot]

Created attachment 316848 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Darin Adler]

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.

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Btw, any idea why the test is failing in ios-simulator-wk2?

[Darin Adler]

Form elements are a lot different on iOS. It’s possible that these cases render a lot differently on iOS and this affects the test.

[alan]

Comment on attachment 316844 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:1161
> +

Having bounding rect code here before calling updateLayoutIgnorePendingStylesheets() is confusing. I know boundingClientRect() will clean the style/renderer tree, but it is highly error prone.

[alan]

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.

What do you mean by elements not having renderer would implement it too? What would they return? -elements with no renderers don't compute client rects.

[Carlos Garcia Campos]

(In reply to Darin Adler from comment #7)
> Form elements are a lot different on iOS. It’s possible that these cases
> render a lot differently on iOS and this affects the test.

Should I just mark it as failure then? I have no idea how to make this work on iOS.

[Carlos Garcia Campos]

(In reply to zalan from comment #8)
> Comment on attachment 316844 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316844&action=review
> 
> > Source/WebCore/dom/Element.cpp:1161
> > +
> 
> Having bounding rect code here before calling
> updateLayoutIgnorePendingStylesheets() is confusing. I know
> boundingClientRect() will clean the style/renderer tree, but it is highly
> error prone.

I agree, if it's not a problem in terms of performance, I guess it's ok to call it twice.

[Carlos Garcia Campos]

(In reply to zalan from comment #9)
> 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.
> 
> What do you mean by elements not having renderer would implement it too?
> What would they return? -elements with no renderers don't compute client
> rects.

What this patch is doing :-) HTMLOption and HTMLOptGroup don't have a renderer, but it's possible to use its select element to get their bounding boxes. So, those elements would override absoluteQuads(). I don't know if there are more cases like this.

[alan]

(In reply to Carlos Garcia Campos from comment #10)
> (In reply to Darin Adler from comment #7)
> > Form elements are a lot different on iOS. It’s possible that these cases
> > render a lot differently on iOS and this affects the test.
> 
> Should I just mark it as failure then? I have no idea how to make this work
> on iOS.
No, you could either come up with a different type of test where you add a dedicated iOS expected file, or ignore this test on iOS and do another one specifically for iOS (I'd rather do the first one though).

[alan]

(In reply to Carlos Garcia Campos from comment #12)
> (In reply to zalan from comment #9)
> > 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.
> > 
> > What do you mean by elements not having renderer would implement it too?
> > What would they return? -elements with no renderers don't compute client
> > rects.
> 
> What this patch is doing :-) HTMLOption and HTMLOptGroup don't have a
> renderer, but it's possible to use its select element to get their bounding
> boxes. So, those elements would override absoluteQuads(). I don't know if
> there are more cases like this.
These are special cases, the typical case when an element does not have a renderer is when the element is display: none.

[Carlos Garcia Campos]

(In reply to zalan from comment #13)
> (In reply to Carlos Garcia Campos from comment #10)
> > (In reply to Darin Adler from comment #7)
> > > Form elements are a lot different on iOS. It’s possible that these cases
> > > render a lot differently on iOS and this affects the test.
> > 
> > Should I just mark it as failure then? I have no idea how to make this work
> > on iOS.
> No, you could either come up with a different type of test where you add a
> dedicated iOS expected file, or ignore this test on iOS and do another one
> specifically for iOS (I'd rather do the first one though).

Ok, I'll add a expected file for iOS then.

[Carlos Garcia Campos]

(In reply to zalan from comment #14)
> (In reply to Carlos Garcia Campos from comment #12)
> > (In reply to zalan from comment #9)
> > > 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.
> > > 
> > > What do you mean by elements not having renderer would implement it too?
> > > What would they return? -elements with no renderers don't compute client
> > > rects.
> > 
> > What this patch is doing :-) HTMLOption and HTMLOptGroup don't have a
> > renderer, but it's possible to use its select element to get their bounding
> > boxes. So, those elements would override absoluteQuads(). I don't know if
> > there are more cases like this.
> These are special cases, the typical case when an element does not have a
> renderer is when the element is display: none.

Ah, ok, I actually meant these special cases in particular not the general case of not having a renderer.

[Simon Fraser (smfr)]

<option> rendering is just weird in general: bug 169039

[Carlos Garcia Campos]

Created attachment 316950 [details]
Updated patch

Added a helper function in the end.

[Build Bot]

Comment on attachment 316950 [details]
Updated patch

Attachment 316950 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4240671

New failing tests:
fast/dom/StyleSheet/detached-sheet-owner-node-link.html

[Build Bot]

Created attachment 316977 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Comment on attachment 316950 [details]
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) });
        }
    ...

[Darin Adler]

Comment on attachment 316950 [details]
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.

[Carlos Garcia Campos]

Created attachment 317218 [details]
Updated patch

Addressed all review comments

[Darin Adler]

Comment on attachment 317218 [details]
Updated patch

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

> Source/WebCore/dom/Element.cpp:1166
> +    if (!selectElement->renderer() || !is<RenderListBox>(selectElement->renderer()))
> +        return std::nullopt;

I think we still need to null-check selectElement here, since ownerSelectElement can return nullptr.

[Carlos Garcia Campos]

(In reply to Darin Adler from comment #24)
> Comment on attachment 317218 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317218&action=review
> 
> > Source/WebCore/dom/Element.cpp:1166
> > +    if (!selectElement->renderer() || !is<RenderListBox>(selectElement->renderer()))
> > +        return std::nullopt;
> 
> I think we still need to null-check selectElement here, since
> ownerSelectElement can return nullptr.

Indeed, good catch.

[Carlos Garcia Campos]

Committed r220313: <http://trac.webkit.org/changeset/220313>

[Radar WebKit Bug Importer]

<rdar://problem/33739164>