inner block is too big, causing focus ring to draw around clipped text Open attached test case, click on the popup to give it focus. Observe that the focus ring is too big.
Created attachment 11523 [details] test case
Created attachment 11525 [details] Fix, creates style first.
Comment on attachment 11525 [details] Fix, creates style first. The point of using a more lightweight clipping system for buttons and menulists was to avoid creating RenderLayers for these controls. In the case of single-line textfields this was unavoidable because we want to be able to auto-scroll to reveal all the contents, but there's no reason to pay this kind of penalty for menu lists or buttons. Buttons use the same lightweight clip model that menulists do and so have the same bugs. Fixing this is very easy and involves subclassing nodeAtPoint to solve the hit testing issues and making a tweak to the outline code to avoid descending into children for blocks with the lightweight clipping model.
Basically we want to formalize the notion of a control clip (kind of like hasOverflowClip but hasControlClip instead), and then patch a number of places in the code to check hasControlClip, e.g., to get overflowWidth/Height right, repaint invalidation right, and outlines right.
Created attachment 12373 [details] Patch that fixes all the issues that kept buttons and popups from hit testing and repainting correctly
Comment on attachment 12373 [details] Patch that fixes all the issues that kept buttons and popups from hit testing and repainting correctly Just listing here stuff we discussed on IRC: Instead of overriding layout() and nodeAtPoint(), add hasControlClip() checks at the end of RenderBlock's implementations. Remove the display check as you said you will: + // Only need to check inline display. + if (noninherited_flags._effectiveDisplay == INLINE && + noninherited_flags._vertical_align != other->noninherited_flags._vertical_align) return Layout; Forgot to negate the last two: - !(inherited_flags._text_decorations == other->inherited_flags._text_decorations) || - !(inherited_flags._force_backgrounds_to_white == other->inherited_flags._force_backgrounds_to_white) || - !(surround->border == other->surround->border) || + inherited_flags._text_decorations != other->inherited_flags._text_decorations || + inherited_flags._force_backgrounds_to_white == other->inherited_flags._force_backgrounds_to_white || + surround->border == other->surround->border ||
Created attachment 12381 [details] Patch take 2 This patch makes the changes mitz suggested. I had to add a controlClipRect method, since all three controls were pushing different clips. In the process of doing this, I noticed opportunities to clean up how list box reported its clip rect factoring in the scrollbar, so I made that so that a lot of base class stuff would just automatically work from JavaScript (like clientWidth/Height). I also implemented scrollLeft/Top/Width/Height on listboxes, so that JS can control the scroll position using pixel values (which we floor to determine the desired option to snap to).
Comment on attachment 12381 [details] Patch take 2 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.
The RenderStyle changes aren't really part of this bug. You can go ahead and land them separately, with a test for vertical-position.
My list box controlClipRect does not include the scrollbar. clientWidth/Height excludes the scrollbar. Note the virtual override of verticalScrollbarWidth in RenderListBox. This override makes the result from the base class clientWidth method correct.
Comment on attachment 12381 [details] Patch take 2 > My list box controlClipRect does not include the scrollbar Sorry, my bad! Since all my other comments were optional, this is a r+.
Fixed in r18819. There's lots of cleanup to do in list box. I copied my scroll function from the scrollToReveal function, so it has all the same issues mitz pointed out.