136467 - [Linux] F7 doesn't always toggle on caret

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Aaron Leventhal]

On Akkana's machine, pressing F7 to turn on caret browsing in content and saying
"Yes" in the confirm dialog, doesn't make the caret visible until you hit a key.

It should automatically turn the caret on

Comment 1

[Aaron Leventhal]

Attachment: When we just received focus, make sure ResetBrowseWithCaret() routine knows it

Akkana will you test this?

Comment 2

[Akkana Peck]

Alas, the caret still doesn't show after applying the patch.

Comment 3

[Aaron Leventhal]

Can any of the Beijing team members see this bug on their machines?

Comment 4

[sairuh (rarely reading bugmail)]

this is what i see using 2002.04.09.09-static comm bits on linux rh7.2:

1. start with caret mode *off*
2. click somewhere (with mouse) in a web page, eg in
http://bugzilla.mozilla.org/ click btwn the "What's" and "all".
3. hit F7.

results: the caret doesn't appear until i hit Tab (which focuses the closest
link, "mozilla.org"). is this what you're seeing?

also, if for (2) i had instead focused a link (focus ring present) then hit F7
to turn on caret browsing, i don't encounter this problem. in that case the
caret immediately appears.

on a related, but tangential note: has any seen the coverse of this bug? ie,
hitting F7 to turn *off* caret browsing, but the caret persisting? (until
reloading or hitting Tab.) i *think* i might've encountered that once, but
cannot remember the platform offhand (wasn't linux, iirc)...

Comment 5

[Akkana Peck]

> hitting F7 to turn *off* caret browsing, but the caret persisting? 

I have seen that (on linux) when I was NOT in caret mode: in other words, I
hadn't typed F7, and I hadn't ever seen the confirmation dialog, yet for some
reason a blinking caret appears in browser content.  Once it appears I can't get
rid of it, but I haven't figured out how to make it appear repeatably (so hadn't
filed a bug).

Comment 6

[Jay Yan]

I will try to reproduce it on my machine.

Comment 7

[Jay Yan]

I tried trunk code on my Solaris machine, the result is as same as comment #4,
but I was confused by comment #5,so I did not test as #5. :-)
Aaron, do you want BeiJing team continue to work on it or you can find more
suitable owner?

Comment 8

[Aaron Leventhal]

Jay, please have the Beijing team look into this.

From Akkana and my tests, we think SetContentCaretEnabled() is getting PR_FALSE
from ResetBrowseWithCaret(), because gLastFocusedDocument != mDocument. We're
not sure why that would be.

Comment 9

[Jay Yan]

[e-mail] is investigating some other bugs about caret, John, can you
take care of this bug at the same time? Thanks

---------This "John" is from BeiJing team, not jgaunt :-)   -----------

Comment 10

[John Sun]

Attachment: v1

If switch on the caret with a dialog, then gLastFocusedDocument=0 after the
dialog
is closed, it's a temporary state for enter here from a callback function, so
make it work incorrectly.

Aaron, pls review the patch. THX!

Comment 11

[Aaron Leventhal]

Thanks for debuggin this.

Instead of 
if (gLastFocusedDocument)
  return SetContentCaretVisible(presShell, mCurrentFocus, *aBrowseWithCaret &&
gLastFocusedDocument == mDocument);
else
  return SetContentCaretVisible(presShell, mCurrentFocus, *aBrowseWithCaret);

How about: 
return SetContentCaretVisible(*aBrowseWithCaret && (!gLastFocusedDocument ||
gLastFocusedDocument == mDocument));

Comment 12

[John Sun]

Attachment: v2

According to Aaron's suggestion.
Make the source more concise.
Aaron, you save sources as money. :-)
pls review. Thanks.

Comment 13

[Aaron Leventhal]

Comment on attachment 95221 [details] [diff] [review]
v2

r=aaronl. thanks!

Comment 14

[John Sun]

jst,
Could you pls sr the small patch?

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 95221 [details] [diff] [review]
v2

Eek, nits in this patch:

>   // Make caret visible or not, depending on what's appropriate
>   if (presShell)
>-    return SetContentCaretVisible(presShell, mCurrentFocus, *aBrowseWithCaret && gLastFocusedDocument == mDocument);
>+      return SetContentCaretVisible(presShell, mCurrentFocus, *aBrowseWithCaret &&

1. The indentation in this file is 2 spaces.  Please don't change that.  Move
the return back over from whence it came.

>+		     (!gLastFocusedDocument || gLastFocusedDocument == mDocument));

2. ACK!  Do not use tab characters in our source.  Please use spaces instead. 
Look at how crappy the indentation is when viewing your patch in Mozilla.

3. When you substitute tabs with spaces, please make sure everything aligns
nicely.  Something like this should be nice:

  return SetContentCaretVisible(presShell, mCurrentFocus,
				*aBrowserWithCaret && (!gLastFocusedDocument ||
						       gLastFocusedDocument ==
mDocument));

Or if that long line bothers you:

  return SetContentCaretVisible(presShell, mCurrentFocus,
				*aBrowserWithCaret &&
				(!gLastFocusedDocument ||
				 gLastFocusedDocument == mDocument));


4. Since you increased the number of lines (not statements) of the |if
(presShell)|, it would be a really good idea to add braces around that block. 
Please do so.


Johnny, anything to add?  :-)

Comment 16

[John Sun]

Attachment: V3

According to Christopher Aillon's suggestion.
Thank Christopher Aillon, I'm a little careless for the the small patch.

Aaron,
Have to bother you to review again.

Johnny,
Seek your sr.

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 95338 [details] [diff] [review]
V3 

r=caillon too, fwiw.

Comment 18

[John Sun]

Aaron & Aillon,
Thanks!

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 95338 [details] [diff] [review]
V3 

sr=jst

Comment 20

[Bolian Yin]

checked in Trunk.

Comment 21

[John Sun]

verified.