Closed Bug 306173 Opened 19 years ago Closed 18 years ago

Caret remains drawn (leaves turd) when switching tabs

Categories

(SeaMonkey :: Tabbed Browser, defect)

Product:
SeaMonkey
Component:
Tabbed Browser
Platform:
PowerPC
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: uriber, Assigned: mark)

References

Details

(Keywords: regression)

Attachments

(1 obsolete file) 

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).
WFM
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050827
Firefox/1.6a1 ID:2005082706
Summary: Caret remains drawn (leavs turd) when switching tabs → Caret remains drawn (leaves turd) when switching tabs
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.
Indeed, backing out the patches for bug 299677 fixes this bug.
Blocks: 299677
Flags: blocking1.8b5?
bryner, can you dig into this for us?
Assignee: nobody → bryner
Flags: blocking1.8b5? → blocking1.8b5+
Flags: blocking1.8b5+ → blocking1.8b5-
Flags: blocking1.9a1?
Attached patch Possible fix (obsolete) — — Splinter Review 
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.
Attachment #199899 - Flags: superreview?(jst)
Attachment #199899 - Flags: review?(bryner)
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.
Flags: blocking1.8rc1?
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?
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.
Would this be better with focusController->GetActive(&active) and a condition on
active before poking focusController any further?
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.
Flags: blocking1.8rc1? → blocking1.8rc1-
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.
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.
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.
(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...
(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?
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).
*** Bug 188517 has been marked as a duplicate of this bug. ***
I can see this bug very often with Firefox 1.5 RC1. It has never occurred before (e.g. in the beta releases).
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).

(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).
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.
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.
Flags: blocking1.8.1?
Flags: blocking1.8.0.3?
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)
Assignee: bryner → mark
Flags: blocking1.8.0.3? → blocking1.8.0.3-
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
Status: NEW → RESOLVED
Closed: 18 years ago
Depends on: 287813
Resolution: --- → FIXED
Attachment #199899 - Attachment is obsolete: true
Attachment #199899 - Flags: superreview?(jst)
Attachment #199899 - Flags: review?(bryner)
Verified FIXED using build 2006-05-07-06 of SeaMonkey trunk under Windows XP with Uri's testcase in comment 0.
Status: RESOLVED → VERIFIED
(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 ;)

Not going to block 1.8.1 for this bug.
Flags: blocking1.8.1? → blocking1.8.1-
Flags: blocking1.9a1?
Reopening per comment 26 and activity since.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
(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.


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.
(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. 


*** Bug 347985 has been marked as a duplicate of this bug. ***
*** Bug 353099 has been marked as a duplicate of this bug. ***
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.
FYI, this bug is still present in the FF 2.0 release, at least on the Mac. For me it has never been away. 
(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 :) )
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: