[webkit-reviews] review denied: [Bug 11598] REGRESSION: NativePopUp: inner block is too big, causing focus ring to draw around clipped text : [Attachment 12381] Patch take 2

[bugzilla-request-daemon at macosforge.org]

mitz at webkit.org has denied mitz at webkit.org's request for review:
Bug 11598: REGRESSION: NativePopUp: inner block is too big, causing focus ring
to draw around clipped text
http://bugs.webkit.org/show_bug.cgi?id=11598

Attachment 12381: Patch take 2
http://bugs.webkit.org/attachment.cgi?id=12381&action=edit

------- Additional Comments from mitz at webkit.org
I'm afraid this patch will re-introduce bug 11133, as your listbox
controlClipRect includes the scrollbar and would allow text to paint on top of
it. The clip in RenderListBox::paintObject was specifically adjusted to exclude
the scrollbar. Not sure about how to solve this. I guess you could just
override paint() altogether.

There are still places in the code that do "style()->font().height() +
optionsSpacingMiddle". I think you should replace them all with itemHeight(),
perhaps inlining it.

This repeats enough times (in existing codes + your patch) that I'd consider
factoring it out:

+    HTMLSelectElement* select = static_cast<HTMLSelectElement*>(node());
+    const Vector<HTMLElement*>& listItems = select->listItems();

static_cast<int> is preferred over (int):

+    return max(clientHeight(), (int)listItems.size() * itemHeight());
+    if (index < 0 || index > (int)listItems.size() - 1 ||
listIndexIsVisible(index))


I don't think you need to involve absolute coords in this:

+	 IntRect rect = absoluteBoundingBoxRect();
+	 m_vBar->setValue(itemBoundingBoxRect(rect.x(), rect.y(), newOffset +
m_indexOffset).y() - rect.y());

You can just do:

+	 m_vBar->setValue(itemBoundingBoxRect(0, 0, newOffset +
m_indexOffset).y());

I consider the snap-to-item scrolling behavior a regression (bug 11134). Also
using listItems.size() for the presumed height is wrong, since the control can
be styled to have a different height (bug 11999).

r- because of the incorrect clipping of list items.