368445 - Focus Outline of Select missing left, top, and bottom

Status: RESOLVED FIXED

Description

[Ronald Tilby]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070127 Minefield/3.0a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070127 Minefield/3.0a2pre

The focus outline (dotted lines) shows only the right side line in simple SELECT elements.

Reproducible: Always

Steps to Reproduce:
1.Load URL (or any page with simple SELECT elements)
2.Tab to the SELECT element
3.Observe that only the right side of the focus outline is shown.
Actual Results:  
Only the right side of the focus outline is visible.

Expected Results:  
The entire focus outline (four dotted lines) should be visible to indicate that the element has focus.

This regressed on 2007-01-17 between Trunk hourly builds 14:08 and 14:32.

Comment 1

[Ronald Tilby]

Created attachment 253035 [details]
Testcase

Testcase Passes using: Trunk 2007-01-17 14:08 Hourly

Testcase Fails using: Trunk 2007-01-17 14:32 Hourly
and Current Nightly
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070127 Minefield/3.0a2pre ID:2007012704 [cairo]

Comment 2

[Phil Ringnalda (:philor)]

Range neatly fits bug 366722.

Comment 3

[Martijn Wargers (zombie)]

I'm seeing this bug with the windowsXP theme, not with the classic theme.

Comment 4

[Daniel Holbert [:dholbert] (away 4/24 - 5/11; use needinfo to be sure I see pings on return)]

I'm not seeing the bug on trunk as of today (2007-05-23), running with default theme under linux.

Comment 5

[Daniel Holbert [:dholbert] (away 4/24 - 5/11; use needinfo to be sure I see pings on return)]

(In reply to comment #4)
> I'm not seeing the bug on trunk as of today (2007-05-23), running with default
> theme under linux.
> 

Oops, cancel that comment -- I didn't notice that this bug is WinXP-only.

Martijn's comment #3 still applies.  I tested the 5/23/07 nightly WinXP build of trunk, and the bug occurs specifically when WinXP is using "Windows XP Style" windows and buttons, not "windows classic" style.

Comment 6

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Is this a drawing order issue?  Does nsComboboxControlFrame::PaintFocus get called too early (so other things paint on top of it)?  Or is the sizing off somehow?

Comment 7

[Alfred Kayser]

It seems to me that the refresh/redraw area for outline changes are not calculated correctly when only the 'outline' style changes. 
If also another style changes (forcing reflow?) than the redraw is better in my experience.

Comment 8

[Chris Pearce (:cpearce)]

Created attachment 282060 [details]
Screen shot comparing FF2 and Minefield-trunk on Windows Vista

This is a screen shot comparing FireFox2 and Minefield on Windows Vista. If you zoom in and look closely, it looks like the combobox's dropdown button is not getting its dark blue/purple border/outline drawn. Is this related? Does this happen on other platforms, or only for Windows Vista?

Comment 9

[Alfred Kayser]

Another way to reproduce focus redraw issues:
Install and select 'Nautipolis' from AMO.
Restart FF.
Open Tools/Options.
Use Tab key to move focus.
See how the focus ring (which is 2 pixels wide, surrounding the widget) gets drawn only partially.
If you move the move over the widgets, the whole focus ring is correctly drawn (this is probably because other style attributes get changes with :hover, such as background). 

It looks like as if the 'outline' draw is not triggered correctly.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

My guess is that what's happening is that certain frame children of the <select> are being given the same style context as the primary frame for the element, which has overflow:-moz-hidden-unscrollable. This clips the content of each such frame to its inner-border-rect. If you remove overflow:-moz-hidden-unscrollable from 'select' in forms.css, I bet the problem goes away. However that is not really the right fix.

Comment 11

[Dão Gottwald [::dao]]

(In reply to comment #10)
> If you remove
> overflow:-moz-hidden-unscrollable from 'select' in forms.css, I bet the problem
> goes away.

Indeed, this looks as expected:

data:text/html,<SELECT style="overflow:visible"><OPTION>Foo</OPTION><OPTION>Bar</OPTION></SELECT>

Comment 12

[Dão Gottwald [::dao]]

(In reply to comment #11)
> data:text/html,<SELECT
> style="overflow:visible"><OPTION>Foo</OPTION><OPTION>Bar</OPTION></SELECT>

data:text/html,<SELECT style="overflow:visible"><OPTION>Foo<OPTION>Bar</SELECT>

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The listbox/combobox-dropdown is an nsListControlFrame, which is a subclass of nsGfxScrollFrame and overrides GetScrollbarStyles, hence ignores its value of 'overflow' for determining its own scrollbar settings. It has one scrollable child frame, an nsSelectsAreaFrame, which actually does not inherit 'overflow' as I had previously hypothesized.

The problem here is that nsFrame::BuildDisplayListForChild, when called on the nsSelectsAreaFrame, is noticing that the parent is overflow:-moz-hidden-unscrollable, and therefore trying to clip the child to the parent's padding-rect. But it computes the padding-rect incorrectly, by taking the parent's border-rect and then subtracting the border taken from the parent's style context (2px on each side). But, on my Mac at least, nsITheme has overridden the parent border to be 1px on each side, so we're actually clipping 1px inside the top, left and bottom of the nsSelectsAreaFrame. (The right escapes because the nsSelectsAreaFrame doesn't extend over to the right edge of the parent, because the scrollbar is in the way.)

So the first thing we need to do is fix ApplyOverflowClipping in BuildDisplayListForChild to use parent->GetPaddingRect, which calls GetUsedBorder to ensure that we compute the padding-rect using overridden border values, if there are any. That's easy.

We should probably also get rid of overflow:-moz-hidden-unscrollable on selects, where it's not really having an effect, and replace it with something more targeted. I'm guessing it's supposed to be preventing the combobox text display and button from overflowing.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Created attachment 282181 [details] [diff] [review]
fix part 1

This fixes ApplyOverflowClipping to use the correct padding-rect, as described in the comment above. Trivial.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems that overflow:-moz-hidden-unscrollable was added to <select> by the reflow branch, so I'm guessing Boris added it. However, I'm not sure why it's needed.

For listboxes, it's superfluous because the listbox is a nsGfxScrollFrame, uses its own scrollbar settings, and clips its kids appropriately.

For comboboxes, it doesn't affect the dropdown at all. nsComboboxControlFrame::Reflow forces the text display frame and the button frame to fit horizontally. They may not fit vertically if the select has a non-auto height --- is that the goal here, to clip the text display frame and the button frame if they overflow vertically?

Comment 16

[David Baron :dbaron: ⌚️UTC-7]

Comment on attachment 282181 [details] [diff] [review]
fix part 1

r+sr+a1.9=dbaron

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in, so marking this fixed.

Comment 18

[Boris Zbarsky [:bz] (no decent commit message means r-)]

nsDisplayBorder::OptimizeVisibility needs a similar change, right?

As for the moz-hidden-unscrollable style, I believe that was the idea yes.  Maybe we should add a comment to that effect....

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Created attachment 282199 [details] [diff] [review]
fix nsDisplayBorder::OptimizeVisibility

Comment 20

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Comment on attachment 282199 [details] [diff] [review]
fix nsDisplayBorder::OptimizeVisibility

r+sr=bzbarsky

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked that in, thanks.