306173 - Caret remains drawn (leaves turd) when switching tabs

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Uri Bernstein]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050824
Firefox/1.6a1

When switching to another tab while the caret is drawn in a text box or
textareas,  the caret remains drawn (but doesn't blink) on the new tab.

Steps to reproduce:
- Open two tabs, one with http://www.google.com and one with about:blank (or any
other mostly-empty page).
- On the Google tab, place the caret in the search field.
- While the caret is drawn (i.e. during the "on" half of the caret blink cycle),
switch to the other tab.
- Notice the caret is there.

This regressed on the trunk between 2005-08-23-07 and 2005-08-24-07. Checkins:
http://tinyurl.com/9ysg4 (surprisingly, none of them are my patches).

Comment 1

[Dave Townsend [:mossop]]

WFM
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050827
Firefox/1.6a1 ID:2005082706

Comment 2

[Uri Bernstein]

This now regressed on the 1.8 branch as well, between 2005-08-27-14 and
2005-08-28-05.

A checkin which matches the regression window both on the trunk and on the
branch is bug 299677.

Comment 3

[Uri Bernstein]

Indeed, backing out the patches for bug 299677 fixes this bug.

Comment 4

[Asa Dotzler [:asa]]

bryner, can you dig into this for us?

Comment 5

[Mark Mentovai]

Attachment: Possible fix

Bug 299677 made this change to Blur methods:

-  SetElementFocus(PR_FALSE);
+  if (ShouldFocus(this))
+    SetElementFocus(PR_FALSE);

That's fine for Focus methods, but it seems wrong to me to do that for Blur. 
The conditions for focusing aren't the same as the conditions for blurring.

The attached patch changes the Blur method for HTML <input> and <textarea>
elements so that it will blur the element if it's focused.  I didn't change any
other elements in the interest of keeping the patch safe, since these are the
only two elements that have carets and exhibit the bug.  I suspect that if this
approach is correct here, then something similar should be done for other
elements.  (Guidance wanted.)

This patch handles the testcases in bug 299677 properly.

Comment 6

[Mark Mentovai]

This bug was blocking1.8+ at one point.  From this end, it looks like it was
minused because nobody wanted to pick it up.  I'm re-requesting blocking for
this ugly turd regression.

Comment 7

[Brian Ryner (not reading)]

So, the reason for the original change was that blurring an element in an
inactive tab would cause focus (real, not latent) to be given to the DOMWindow
in that tab.  What prevents that from happening if we revert that change?

Comment 8

[Mark Mentovai]

I'm seeing that when you switch tabs, Blur() is called on the focused element. 
This is the blur that's being lost, at least on the Mac, because ShouldFocus()
is returning false.  With this change, once the element is blurred, it can't be
blurred again until it's focused, and it can't be focused unless it ShouldFocus,
that is, its tab is active.

Comment 9

[Mark Mentovai]

Would this be better with focusController->GetActive(&active) and a condition on
active before poking focusController any further?

Comment 10

[Asa Dotzler [:asa]]

not going to block on this but if it lands on the trunk, is verified and you
give us a pretty positive risk analysis, we'll consider approving the patch.

Comment 11

[Johnny Stenback (:jst)]

Shouldn't we be fixing the Mac caret to behave like it does on Windows and Linux
(I don't see this bug on either) rather than changing our blur() behavour,
especially on the branch.

Comment 12

[Brian Ryner (not reading)]

jst: I don't think it's that the caret behaves differently, it's likely because
of the OS double-buffering on Mac that causes us not to get any additional
invalidates on the area where the caret was drawn.

It seems like we should fix the order of events when switching tabs so that when
switching tabs, the focused content is blurred _before_ the old tab is
considered inactive.  Of course, that would allow the blur to focus something
else in the old tab, so maybe we need a "you can blur but you can't focus"
state... ugh.

Comment 13

[Mark Mentovai]

This discussion assumes that ShouldFocus() was the right thing to do in Blur()
methods in bug 299677, but I'm not convinced that's the case.  The conditions
under which Focus is appropriate aren't identical to the conditions under which
Blur is.

From what I've seen, this patch leaves 299677 fixed and also fixes the Mac
caret, and the conditions are closer to what I'd expect of a Blur.

I thought that the blur and the tab switching would all be handled in
tabbrowser.xml, but it looks like the blur comes before anything significant in
updateCurrentBrowser, and the onselect occurs after the tab is switched.  More
importantly, I don't see how ShouldFocus() is allowing this blur to succeed on
any platform since 299677 landed.

Comment 14

[Johnny Stenback (:jst)]

(In reply to comment #13)
> This discussion assumes that ShouldFocus() was the right thing to do in Blur()
> methods in bug 299677, but I'm not convinced that's the case.  The conditions
> under which Focus is appropriate aren't identical to the conditions under which
> Blur is.

Right or wrong, this is *not* something that I feel comfortable with changing
this late in this product cycle. Other changes that have less of an impact and
still fixes this problem I'd be ok with, but really, I don't want to change our
focus or blur behaviour any more for 1.5...

Comment 15

[Mark Mentovai]

(In reply to comment #14)
> Right or wrong, this is *not* something that I feel comfortable with changing
> this late in this product cycle. Other changes that have less of an impact and
> still fixes this problem I'd be ok with, but really, I don't want to change our
> focus or blur behaviour any more for 1.5...

That's fair.  Would you be comfortable with trying to attack this by making the
blur occur before the tab's visibility goes false?

Comment 16

[Uri Bernstein]

See also bug 188517, which describes the same problem, on the Suite, back in Mozilla 1.2.
This problem the disappeared from the Suite sometime between 1.8a2 and 1.8a3, only to reappear on 2005-08-24 (when it first appeared on Firefox).

Comment 17

[Stewart Gordon]

*** Bug 188517 has been marked as a duplicate of this bug. ***

Comment 18

[Vincent Lefevre]

I can see this bug very often with Firefox 1.5 RC1. It has never occurred before (e.g. in the beta releases).

Comment 19

[Ginn Chen]

This patch is a clue.
Also I think it's not enough.
When you turn on caret browsing by pressing F7, caret can leave turd everywhere (not only text area or input element).

Comment 20

[Uri Bernstein]

(In reply to comment #19)
> When you turn on caret browsing by pressing F7, caret can leave turd everywhere
> (not only text area or input element).

For the record, this was bug 300125 (now fixed).

Comment 21

[Martijn Wargers (dead)]

Actually, I came up with the same idea in a different bug (I forgot about this patch), see bug 330705. The patch there is basically the same principle as this patch.

Comment 22

[Mark Mentovai]

The visual defect still aggravating on the 1.8.0 branch, and it is a regression from aviary 1.0.x.  It would be nice to fix this for 1.8.0.3.  Barring that, I think it should definitely be fixed for 1.8.1.

Comment 23

[Daniel Veditz [:dveditz]]

Given how stale this bug and patch are this doesn't seem like a blocker. If you get traction on reviews you can request branch approval for checkin along with appropriate explanation of risk vs. benefit. (On its face, a visual defect fix argues for nearly zero risk tolerance)

Comment 24

[Uri Bernstein]

Fixed by the fix for bug 287813.

WFM with
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060417
Firefox/3.0a1 ID:2006041723

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2006-05-07-06 of SeaMonkey trunk under Windows XP with Uri's testcase in comment 0.

Comment 26

[Martijn Wargers (dead)]

(In reply to comment #25)
> Verified FIXED using build 2006-05-07-06 of SeaMonkey trunk under Windows XP
> with Uri's testcase in comment 0.

Well, this was a Mac specific bug, afaik, so I think it's hard to verify under Windows XP ;)

Comment 27

[Darin Fisher]

Not going to block 1.8.1 for this bug.

Comment 28

[Stewart Gordon]

Reopening per comment 26 and activity since.

Comment 29

[Uri Bernstein]

This was, in fact, fixed. Verified on:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060617 Minefield/3.0a1

If you still see a problem, please report a separate bug.

Comment 30

[Wayne Woods]

(In reply to comment #29)
> If you still see a problem, please report a separate bug.

I believe this was fixed for the trunk, but NOT the 1.8 branch, since the check-in for bug 287813 was only intended for the trunk (see bug 287813 comment 59). Any solution for the branch will probably have to be a safer hack job. As Uri says, file a new bug (specifying it as for the branch only), then log a comment over at bug 287813 and see if someone can help you.

Comment 31

[Stewart Gordon]

Sorry, slipped up a bit.  But at least it's back from having been verified by someone who's in no position to do so.

Comment 32

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #31)
> Sorry, slipped up a bit.  But at least it's back from having been verified by
> someone who's in no position to do so.

My bad. 

Comment 33

[Mark Mentovai]

*** Bug 347985 has been marked as a duplicate of this bug. ***

Comment 34

[Phil Ringnalda (:philor)]

*** Bug 353099 has been marked as a duplicate of this bug. ***

Comment 35

[Wayne Woods]

I don't think anyone spun off a bug for the 1.8 branch version of this (at least I couldn't find one). So I opened a new one: bug 355712.

Comment 36

[Christiaan Hofman]

FYI, this bug is still present in the FF 2.0 release, at least on the Mac. For me it has never been away. 

Comment 37

[Wayne Woods]

(In reply to comment #36)
> FYI, this bug is still present in the FF 2.0 release, at least on the Mac. For
> me it has never been away. 

As per comment 35... see bug 355712 ( Fx 2.0 is on the Gecko 1.8 branch :) )