417124 - [10.4] Aqua focus ring is cut off after form autocomplete goes away

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Jesse Ruderman]

Attachment: screenshot

Steps to reproduce:
1. Load http://www.google.com/
2. Type "fooooooo" (one letter at a time)

(When you type 'f', a form autocomplete menu should appear.  Once you type enough 'o's for there to be no more matches, it should disappear.)

Result: When it disappears, some of the aqua focus ring (the part that was below the autocomplete menu?) is missing.  Seems like a painting invalidation bug.

Nominating because this bug is pretty visible, and wasn't a problem before bug 415466 was fixed a few days ago.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

See also bug 416929, which also seems to be about poor painting invalidation/repainting with a text widget.

Comment 2

[Josh Aas]

I cannot reproduce this. The focus ring remains intact when the autocomplete dialog disappears on account of no more matches.

Jesse - is this always reproducible for you?

Comment 3

[Jesse Ruderman]

Almost always.  I noticed that it doesn't happen if the autocomplete menu goes away due to pressing Esc, fwiw.

Comment 4

[Josh Aas]

Can anyone else besides Jesse reproduce this?

Comment 5

[Marcia Knous [:marcia]]

I am not able to reproduce this using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008021404 Minefield/3.0b4pre.

Comment 6

[Josh Aas]

Jesse - are you on 10.4 or 10.5?

Comment 7

[Phil Ringnalda (:philor)]

I can reproduce it on 10.4, at least 95% of the time.

Comment 8

[Jesse Ruderman]

I'm on 10.4.

Comment 9

[Josh Aas]

That seems to be the key - adding [10.4] to the summary.

Comment 10

[Josh Aas]

Attachment: fix v1.0

Not awesome, but looks like that is what we're up against here. See comment in the patch.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 303389 [details] [diff] [review]
fix v1.0

This is overkill (possibly causing two windows to be completely
repainted).  But I doubt very much that it will ever cause any
noticeable delay (popup windows don't get repeatedly and quickly shown
and hidden).  And it certainly has the virtue of simplicity.

I haven't tested this ... but that didn't seem necessary.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

(By the way, I also see this problem on Tiger but not on Leopard.)

Comment 13

[Josh Aas]

landed on trunk

Comment 14

[Phil Ringnalda (:philor)]

Ack, cure vastly worse than the disease.

I don't know to what extent dialup (or the equivalent, download a couple of ISOs while letting a Bittorrent app saturate your connection) is required, but for me, loading a bug page with this patch in flashes the entire window between five and ten times - as a first guess, a <select> would be one of the things that hides a popup.

Comment 15

[Boris Zbarsky [:bzbarsky]]

So... several issues here.  The code structure is:

517   if (bState && !mBounds.IsEmpty()) {
...
606   else {
         // roll up stuff
711   }

Obvious issues:

* Treating bState && mBounds.IsEmpty() as equivalent to !bState is weird.  Shouldn't we just bail out early if bState && mBounds.IsEmpty() instead?
* If bState matches out current state, this method should be a no-op.  It most certainly is not.  This might be the case we're hitting here, but I doubt it.
* If !bState and mBounds.IsEmpty(), it doesn't make any sense to force the repaint.  I bet that's the case we're hitting here.

We have various places that assume that Show() with the existing boolean is a reasonably fast operation, and that hiding something we never really showed is pretty fast.  This code is violating both assumptions.

Comment 16

[Jesse Ruderman]

The entire browser chrome now flashes the first time I use awesomecomplete in a window.

Comment 17

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

Josh, you'd better back this out.

Comment 18

[Josh Aas]

Strange that I never saw that in my testing, backed out. I'll probably have a new patch soon.

Comment 19

[Håkan Waara]

Boris comment 15 reminded me of bug 392813 which is trivial to fix, and this might be the right time to do it, see below.

The method is |nsCocoaWindow::Show(PRBool aShow)| which is called on us to show/hide a window. 

Now, this method is often (always, I think) called multiple times in sequence with aShow set to the same value. This, one could argue, is a bug... 

Anyway, the problem with our code is that all the initialization code is re-executed every time, even though we're already showing the window or it is already hidden!

This is potentially harmful as it is likely to get us (or Cocoa) into an inconsistent state, and may have been what was causing Phil Ringnalda's window to flash multiple times in comment 14? Just a hunch.

Comment 20

[Josh Aas]

Attachment: fix v2.0

This basically includes hwaara's fix for bug 392813. Works fine for me,  I tested with some complex sheet test cases among other things. I was able to reproduce the page flashing reported in the bug and this gets rid of it for me.

Comment 21

[Håkan Waara]

Comment on attachment 303928 [details] [diff] [review]
fix v2.0

Wanna remove mWindow as well while you're at it? (Suggestion from the same bug and smichaud seemed to agree.) Basically we can use [mWindow isVisible] instead of this cached value in all places. It's just a possible source for error.

I think bState is a stupid non-descriptive argument name, wanna rename it to aShow or so?

Comment 22

[Håkan Waara]

Sorry, of course I meant "Wanna remove mVisible", and not mWindow :-)

Comment 23

[Josh Aas]

Comment on attachment 303928 [details] [diff] [review]
fix v2.0

Right now I want to be as conservative as possible, so I don't want to make any more changes than are already in this patch.

Comment 24

[Josh Aas]

landed on trunk

Comment 25

[Josh Aas]

backed out

chrome://mochikit/content/browser/toolkit/components/places/tests/browser/browser_bug413985-httprefresh.js
	FAIL - Visit count is what we expect - Got 2, expected 1 - chrome://mochikit/content/browser/toolkit/components/places/tests/browser/browser_bug413985-httprefresh.js

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Bah, that is the test that started failing for me when I checked in the patch for bug 392322... Think it's a fragile test.

Comment 27

[Josh Aas]

Håkan - iirc, one reason we can't get rid of mVisible is that sibling sheets that are hidden because only one can show at a time should be considered visible. The OS call would say that they are not visible. I do agree we could simplify that logic some time though.

Comment 28

[Håkan Waara]

(In reply to comment #27)
> Håkan - iirc, one reason we can't get rid of mVisible is that sibling sheets
> that are hidden because only one can show at a time should be considered
> visible. The OS call would say that they are not visible. I do agree we could
> simplify that logic some time though.

Then we could use it in only that place for sheets (and use [mWindow isVisible] in all other places), and rename it to mIsSheetVisible to make the usage more explicit?

Comment 29

[Josh Aas]

That is basically what I meant by simplify the logic sometime.

Comment 30

[Håkan Waara]

Sounds good to me. Let me know if you need a quick r+ for such changes.

Comment 31

[Josh Aas]

Attachment: fix v3.0

This includes logic cleanup, things should be much more clear now.  The mochitest still fails, I can repro locally, working on that now.

Comment 32

[Josh Aas]

Word on the street is that the failing test has nasty timing issues and it will be disabled today.

Comment 33

[Håkan Waara]

Comment on attachment 304236 [details] [diff] [review]
fix v3.0

Wanna change bState to aShow while you're at it?

Comment 34

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

Comment on attachment 304236 [details] [diff] [review]
fix v3.0

         [mWindow setAcceptsMouseMovedEvents:NO];
-
+        

Don't add trailing whitespace (happens twice)

Comment 35

[Josh Aas]

Attachment: fix v3.1

This is what I checked in. Just whitespace changes.

Comment 36

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

I think this caused bug 503391.