Summary: | getClientRects doesn't work with list box option elements | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||
Component: | DOM | Assignee: | 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
Carlos Garcia Campos
2017-08-01 04:07:23 PDT
Created attachment 316844 [details]
Patch
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 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 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. (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. Btw, any idea why the test is failing in ios-simulator-wk2? 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 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 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. (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. (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. (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. (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). (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. (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. (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. <option> rendering is just weird in general: bug 169039 Created attachment 316950 [details]
Updated patch
Added a helper function in the end.
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 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 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 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. Created attachment 317218 [details]
Updated patch
Addressed all review comments
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. (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. Committed r220313: <http://trac.webkit.org/changeset/220313> |