[Linux] F7 doesn't always toggle on caret

VERIFIED FIXED

Status

()

VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: aaronlev, Assigned: aaronlev)

Tracking

({access})

Trunk
x86
Linux
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

17 years ago
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
(Assignee)

Updated

17 years ago
Keywords: access
(Assignee)

Comment 1

17 years ago
Created attachment 78435 [details] [diff] [review]
When we just received focus, make sure ResetBrowseWithCaret() routine knows it

Akkana will you test this?

Comment 2

17 years ago
Alas, the caret still doesn't show after applying the patch.
(Assignee)

Comment 3

17 years ago
Can any of the Beijing team members see this bug on their machines?
Blocks: 127812
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

17 years ago
> 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

17 years ago
I will try to reproduce it on my machine.

Comment 7

17 years ago
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?
(Assignee)

Comment 8

17 years ago
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

17 years ago
john.sun@sun.com 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

16 years ago
Created attachment 95206 [details] [diff] [review]
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!
(Assignee)

Comment 11

16 years ago
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

16 years ago
Created attachment 95221 [details] [diff] [review]
v2

According to Aaron's suggestion.
Make the source more concise.
Aaron, you save sources as money. :-)
pls review. Thanks.
Attachment #78435 - Attachment is obsolete: true
Attachment #95206 - Attachment is obsolete: true
(Assignee)

Comment 13

16 years ago
Comment on attachment 95221 [details] [diff] [review]
v2

r=aaronl. thanks!
Attachment #95221 - Flags: review+

Comment 14

16 years ago
jst,
Could you pls sr the small patch?
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

16 years ago
Created attachment 95338 [details] [diff] [review]
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.
Attachment #95221 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #95338 - Flags: review+

Comment 18

16 years ago
Aaron & Aillon,
Thanks!

Updated

16 years ago
Attachment #95338 - Flags: superreview+

Comment 20

16 years ago
checked in Trunk.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 21

16 years ago
verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.