[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