278197 - Ctrl+HOME/END doesn't work correctly on www.google.com and www.microsoft.com

Status: RESOLVED FIXED

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

Description

[Ginn Chen]

Turn on caret browsing

1)Ctrl+HOME doesn't work on www.google.com and www.microsoft.com

2)On www.google.com
Ctrl+END moves the caret to the beginning of "©2005 Google -
Searching 8,058,044,651 web pages"
should be the end of "©2005 Google -Searching 8,058,044,651 web pages"

Comment 1

[Ginn Chen]

Attachment: patch

Comment 2

[Ginn Chen]

Comment on attachment 171113 [details] [diff] [review]
patch

always put the caret before img element is not a good idea for CTRL+END

Comment 3

[Ginn Chen]

Attachment: patch v2

Comment 4

[Eyal Rozenberg]

Note that the behavior of this patch needs to be tested not just for browsing
websites, but also for composing HTML and text mail messages.

Comment 5

[Tobias Fischer]

In Reply to Comment #4 I can confirm this behavior for Compose Text-Messages in
MailNews using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b)
Gecko/20050112 Mnenhy/0.7.1 {Build ID: 2005011221}, not only for Web-Sites.

Comment 6

[Eyal Rozenberg]

(In reply to comment #5)

I said the _patch_ needs to be tested in composer as well as in the browser. The
bug is valid, of course.

Comment 7

[Ginn Chen]

(In reply to comment #4)
> Note that the behavior of this patch needs to be tested not just for browsing
> websites, but also for composing HTML and text mail messages.

Mostly works fine.

The only problem is when editing www.microsoft.com, Ctrl+END doesn't work.
nsFrame::GetLastLeaf(GetPresContext(), &resultFrame);
returns an anchor element, but in navigator, it returns __moz_text.

I doubt we should fix it here.
It's a rare case, and GetLastLeaf should return __moz_text in anchor element but
not an anchor.

FYI: before my patch, press Ctrl+END when editing www.microsoft.com causes
mozilla core dump 

Comment 8

[Boris Zbarsky [:bzbarsky]]

Note that the crash mentioned in comment 7 may well be bug 277306

Comment 9

[Kathleen :Brade]

I am concerned about "patch v2."  Specifically, the core editor relies heavily
on <br> functioning in certain ways.  About 20 months ago there was a huge
effort to fix problems with selection and key press events that caused the
editor to get into bad states.  I'd hate to see those regress without even
realizing it (until much later when someone files an Editor: Core bug and the
editor people wouldn't necessarily know about this change at all.  *PLEASE* test
Composer thoroughly before landing "patch v2."

Comment 10

[Kathleen :Brade]

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

Comment 11

[Aaron Leventhal]

Comment on attachment 171124 [details] [diff] [review]
patch v2

I'm not qualified to review this patch. I think Brade is qualified to review,
based on her comments.

Comment 12

[Ginn Chen]

With patch v2,

1- Open a blank document in composer
2- Created a default 2x2 table in Composer 
3- clicked in the bottom-right cell (so that blinking caret and cell resizing
grippies get visible, rendered) 
4- Typed in Ctrl+End 

Caret goes to left of the table.
It's still incorrect.

Comment 13

[Ginn Chen]

Attachment: patch v3

If a page ends by <table>, there will be some anonymous elements, e.g.
mozTableAddRowAfter.
Let GetLastLeaf ignores anonymous elements.
(Q: Do anonymous elements only exist at the end of page? I didn't find them
before non-anonymous elements.)
BTW: I removed a line can never be reached.

With patch v3,
testcase in Comment #12 is solved.

But Ctrl+End still doesn't work while editing www.microsoft.com.
It's a <map> element issue.
In composer, www.microsoft.com ends by <map> elements.
Since currently we don't support Caret Browsing with <map> element,
I don't want to solve it in this bug.
(In browser, the last <map> has a next sibling frame -- __moz_text, so Ctrl+End
works.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

> (Q: Do anonymous elements only exist at the end of page? 

Absolutely not.  They exist, at the moment, in various form controls (eg <input
type="file"> or <input>), in CSS generated content, in XTF-generated content,
and so forth.

I'm not sure what the exact behavior you want for <input type="file"> here is
(to focus the button or to focus the input as a whole), but it's worth checking
with this patch.

I'm also not sure whether composer handles CSS generated content at all
gracefully, but that's also worth checking.

Comment 15

[Ginn Chen]

Patch v3 works well with <input type="file"> and <input> in both browser and
composer.
Caret displays at end of the "Browse..." button or textbox without focus it.

The <input type="file"> or <input> always has a next sibling frame __moz_text.

Testcases: 
<body>
<input type="file">
</body>

<body>
<input>
</body>

Comment 16

[Boris Zbarsky [:bzbarsky]]

What about:

  <body><input type="file"></body>

?

Comment 17

[Ginn Chen]

(In reply to comment #16)
> What about:
> 
>   <body><input type="file"></body>
> 
> ?

Caret displays in textbox without focus (ie you can't input)
I think it's good enough.
"Caret displays in form control without focus" is another bug in my plate.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Did you test the CSS generated content too, btw?

Comment 19

[Ginn Chen]

(In reply to comment #18)
> Did you test the CSS generated content too, btw?

Yes, I did. Works for me, no regression.

Comment 20

[Kathleen :Brade]

Comment on attachment 171728 [details] [diff] [review]
patch v3

I'm not sure I'm the right person to review this code; certainly you'll need a
module owner to approve.

I'm concerned about this block:
+    nsIFrame* siblingFrame;
+    nsIContent* content;
+    while ((siblingFrame = child->GetNextSibling()) &&
+	    (content = siblingFrame->GetContent()) &&
+	    !content->IsNativeAnonymous())
+      child = siblingFrame;

I'd like to see content initialized to null.  I'd also like to see a null check
after content = siblingFrame->GetContent() unless that is guaranteed to always
return a valid pointer (prior to the IsNativeAnonymous call).

I don't think your code introduces this problem but shouldn't *aFrame be
initialized to null before entering the while(1) loop?	It seems like
GetLastLeaf could return without setting it to null.

r=brade

Comment 21

[Ginn Chen]

Thank you, brade!

I think |content| needn't to be initialized, because we always give it a value
in while-clause.(content = siblingFrame->GetContent())
And we don't need null check for it, because the evaluation is in while-clause.
If it is null, it will break out.

aFrame is an in/out parameter, I think we don't need set it to null.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 171728 [details] [diff] [review]
patch v3

sr=bzbarsky if you add a detailed comment about why the IsNativeAnonymous check
is there...

Comment 23

[Ginn Chen]

Attachment: patch checked in

Checking in nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.541; previous revision: 3.540
done

Comment 24

[Ginn Chen]

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

Comment 25

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381