Bug 175016

Summary: getClientRects doesn't work with list box option elements
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, buildbot, cdumez, cmarcelo, darin, dbates, esprehn+autocc, hyatt, jonlee, kangil.han, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 174710    
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Updated patch
buildbot: commit-queue-
Archive of layout-test-results from ews113 for mac-elcapitan
none
Updated patch darin: review+

Description Carlos Garcia Campos 2017-08-01 04:07:23 PDT
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.
Comment 1 Carlos Garcia Campos 2017-08-01 04:12:23 PDT
Created attachment 316844 [details]
Patch
Comment 2 Build Bot 2017-08-01 05:38:38 PDT
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
Comment 3 Build Bot 2017-08-01 05:38:39 PDT
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
Comment 4 Darin Adler 2017-08-01 09:03:09 PDT
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.
Comment 5 Carlos Garcia Campos 2017-08-01 09:23:26 PDT
(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.
Comment 6 Carlos Garcia Campos 2017-08-01 09:27:18 PDT
Btw, any idea why the test is failing in ios-simulator-wk2?
Comment 7 Darin Adler 2017-08-01 09:29:15 PDT
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.
Comment 8 zalan 2017-08-01 09:30:10 PDT
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.
Comment 9 zalan 2017-08-01 09:35:42 PDT
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.
Comment 10 Carlos Garcia Campos 2017-08-01 09:59:41 PDT
(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.
Comment 11 Carlos Garcia Campos 2017-08-01 10:00:32 PDT
(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.
Comment 12 Carlos Garcia Campos 2017-08-01 10:02:55 PDT
(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.
Comment 13 zalan 2017-08-01 10:06:12 PDT
(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).
Comment 14 zalan 2017-08-01 10:09:25 PDT
(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.
Comment 15 Carlos Garcia Campos 2017-08-01 10:20:46 PDT
(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.
Comment 16 Carlos Garcia Campos 2017-08-01 10:21:28 PDT
(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.
Comment 17 Simon Fraser (smfr) 2017-08-01 22:20:16 PDT
<option> rendering is just weird in general: bug 169039
Comment 18 Carlos Garcia Campos 2017-08-02 01:57:46 PDT
Created attachment 316950 [details]
Updated patch

Added a helper function in the end.
Comment 19 Build Bot 2017-08-02 11:15:26 PDT
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
Comment 20 Build Bot 2017-08-02 11:15:28 PDT
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
Comment 21 Darin Adler 2017-08-03 21:53:01 PDT
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) });
        }
    ...
Comment 22 Darin Adler 2017-08-03 21:57:24 PDT
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.
Comment 23 Carlos Garcia Campos 2017-08-03 23:41:22 PDT
Created attachment 317218 [details]
Updated patch

Addressed all review comments
Comment 24 Darin Adler 2017-08-04 09:57:21 PDT
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.
Comment 25 Carlos Garcia Campos 2017-08-05 00:33:36 PDT
(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.
Comment 26 Carlos Garcia Campos 2017-08-05 01:08:27 PDT
Committed r220313: <http://trac.webkit.org/changeset/220313>
Comment 27 Radar WebKit Bug Importer 2017-08-05 01:09:49 PDT
<rdar://problem/33739164>