424873 - Radio and checkbox form controls are missing part of the focus outline

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Reed Loden [:reed]]

Attachment: screenshot of top and left focus outline only

Due to recent patch by dbaron (I believe), form controls such as radios and checkboxes are missing the focus outline on the right and bottom sides of the control. The focus outline appears fine on the top and left. Very rarely, it's possible to get the right and bottom outlines to show (not sure how), but when they do, they won't go away once they are shown.

Comment 1

[Reed Loden [:reed]]

Attachment: screenshot of bottom and right focus outline stuck after unfocusing

Comment 2

[Damon Sicore (:damons)]

+'ing w/ P2.  Reed, can you please attach a testcase?

Comment 3

[Jim Blandy :jimb]

I took this bug in "good volunteer" spirit, but I think I'm going to decline it, as ActionMonkey needs me more, and there are others who could almost certainly fix this more quickly than I.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

It looks like this bug only reproduces when there's enough content to require a vertical scrollbar.

This testcase just has a checkbox and then a decently tall div, to necessitate a vertical scrollbar.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: reference 1

This reference case is just like the testcase, except with a much-shorter div, so there's no vertical scrollbar.  It doesn't reproduce the bug on my machine.

Comment 6

[Daniel Holbert [:dholbert]]

A regression range would be very helpful.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 312137 [details]
testcase 1

>When you click this checkbox, only the upper-right
>corner of focus outline is visible.

Oops -- s/upper-right/upper-left :)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think reed said it was bug 402940.

Comment 9

[Daniel Holbert [:dholbert]]

Yup -- I just tested the nightlies on either side of that checkin (2008030704 - 2008030804), and that is indeed when the regression happened.

Marking dependency & removing qawanted.

Comment 10

[Ian Spence]

So I can see the cause but not the solution

We set DrawRect to a square with sides of length indicator_size
We then set ClipRect to DrawRect
We paint a square box of indicator_size + (2 * indicator_spacing)

So we paint a box that is always bigger than the clip area, hence the clipping.

Personally I would just undo the following part of attachment 307850 [details] [diff] [review]

>@@ -1088,8 +1109,8 @@ nsNativeThemeGTK::GetMinimumWidgetSize(n
>       }
> 
>       // Include space for the indicator and the padding around it.
>-      aResult->width = indicator_size + 2 * indicator_spacing;
>-      aResult->height = indicator_size + 2 * indicator_spacing;
>+      aResult->width = indicator_size;
>+      aResult->height = indicator_size;
>       *aIsOverridable = PR_FALSE;
>     }
>     break;

That being said, I'm swamped with work and won't be able to test that myself

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The clip area should be increased by the value returned from GetExtraSizeForWidget.  Is that not happening?

Comment 12

[u294409]

Reed, the with some GTK engine, which styles checkboxes. With Clearlooks and Nodoka, the checkbox/radiob. instead of label has its focus (semi-transparent rounded rectangle), while label has no focus.

What you filed is true, but this is bug in bug. First you need to make Firefox draw focus on checkbox/radiob. on websites appropriately.

Comment 13

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

Seems to be just an invalidation bug. I should be able to nail it

Comment 14

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

Attachment: fix

The problem is that nsNativeThemeGTK::GetWidgetOverflow is returning a rect based on the frame's current size. This size is stale, in fact in the first reflow it's always 0,0, so the overflow area computed is way too small. We need FinishAndStoreOverflow to pass in the new size so that the theme code can use it.

The reason the testcase is fragile is that if the frame gets reflowed more than once, the subsequent reflow(s) get the correct size and things are OK.

I thought I saw this come up before in another bug, but I can't remember where. Anyway it needs to be fixed and the fix is very straighforward.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 312852 [details] [diff] [review]
fix

r+sr=dbaron, but please rename the parameter from aResult to aOverflowRect or something like that, and put a big /* INOUT */ comment in nsITheme.h.

Comment 16

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

Checked in