263309 - PresShell::CompleteMove simulates a mouse click and assumes LTRness

Status: RESOLVED FIXED

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

Description

[Eyal Rozenberg]

CompleteMove is run for handling Ctrl+Home or Ctrl+End keypresses. What it 
currently does is to simulate a click at the top left or the bottom right of 
the scrolled frame. This is wrong, both because there is no real reason for 
keypresses to simulate mouse clicks - CompleteMove should run some generic 
caret position setting code - and because some of the content may be RTL rather 
than LTR, in which case the beginning might be at the top right and the end 
might be at the bottom left (independently of each other of course).

Comment 1

[Eyal Rozenberg]

Attachment: draft patch

Here's a buggy alternate implementation of CompleteMove. This still uses
HandleClick() ; the code in HandleClick should probably be moved to a separate
function with HandleClick() as an inline wrapper of it, so that CompleteMove
can use it 'cleanly'. I am unfamiliar with this part of Mozilla, so I will need
some guidance and advice as to how to avoid 'dummy' frames at the end of the
content, how to get the relevant offset, etc.

Comment 2

[Eyal Rozenberg]

Attachment: patch v2

This version fixes the obvious bugs of the draft patch. Only tested it with
Thunderbird, though (i.e. not with caret browsing).

Comment 3

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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

seems ok to me from an editor's perspective but I am rally not a layout expert
and would like to defer to roc.
you should test in Midas and browser's caret browsing mode.

Comment 4

[Eyal Rozenberg]

Seems to work 'fine' with caret browsing (that is to say, gets to the same
places you can get to using the 4 arrow keys.

Seems to work 'fine' with Midas (using the demo at
http://www.mozilla.org/editor/midasdemo/) - fine considering the fact that
switching from 'Normal' to 'Paragraph' isn't working all that great

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Don't take an aPresContext parameter. You can get the prescontext from the
nsBlockFrame. Also, you might as well declare it as taking an nsBlockFrame
parameter instead of nsIFrame. (Declare "class nsBlockFrame;" in nsFrame.h so
you don't need to pull in nsBlockFrame.h there.) And why not just return an
nsPeekOffsetStruct as a direct result? In fact, why not make this an actual
method of nsBlockFrame?

nsPeekOffsetStruct nsBlockFrame::GetExtremeCaretPosition(PRBool aStart)

looks much nicer :-)

+      // code shamelessly copied from nsHTMLContentSerializer.cpp

This looks absolutely evil. This code should be extracted into a function
somewhere, shared with nsHTMLContentSerializer.cpp, and marked EVIL.

+    // XXX perhaps this is impossible after using GetFirstLeaf/GetLastLeaf...

It should be; take this code out.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

let's see a new patch :-)

Comment 7

[Eyal Rozenberg]

Attachment: patch v3

- expanded the special-case to all br tags
- made changes as per roc's suggestion
- no real code in common with nsHTMLContentSerializer.cpp

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right now I think the line stuff isn't doing anything and you could just use
GetFirstLeaf/GetLastLeaf. You may want to try taking it out and testing.

I think setting mContentOffset to 0 if aStart is true is wrong. For example, it
could be that the frame in question is a text frame that is a continuation for
some chunk of text that started earlier (for example in an earlier column or
page). In that case you need to get the starting content offset from the frame.

Comment 9

[Eyal Rozenberg]

Should the code of GetExtremeCaretPosition notice when elements have
style="visibility: hidden" ?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Probably not.

Comment 11

[Eyal Rozenberg]

Attachment: patch v4

roc: ok, no accounting for "visibility:hidden".

Patch v4:
- no more line navigation code... now only using GetFirstLeaf and GetLastLeaf
- GetExtremeCaretPosition is now a method of nsIFrame only
- made GetFirstLeaf, GetLastLeaf public
- using GetOffsets() instead of assuming

Comment 12

[Simon Montagu :smontagu]

Checked in.

Comment 13

[Ginn Chen]

After this patch, with caret browsing on, Ctrl+HOME doesn't work on
www.google.com, and Ctrl+END moves the caret to the beginning of "©2005 Google -
Searching 8,058,044,651 web pages"

I think it's a regression.

Comment 14

[Eyal Rozenberg]

Ginn, you're right; however, I think you should open another bug, because this 
one was about the fact that Ctrl+Home simulated a mouse click, which now no 
longer happens.

Also, part of the cause might be the way HandleClick() works.

Also, HandleClick should be renamed to HandleCaretPositioning() , but that's a 
different bug as well.

Finally, I'm really busy and don't have time to work on this problem.

Comment 15

[Ginn Chen]

To solve Ctrl+End issue,

+  PRInt32 start, end;
+  nsresult rv;
+  rv = GetOffsets(start,end);

should be
   rv = resultFrame->GetOffsets(start,end);

I'll file a new bug for it.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Er... "don't have time to work on this problem" for a regression you caused?  Am
I missing something?

For what it's worth, this checkin also caused crash bug 277306, which I've taken
the liberty of assigning to the author of this patch.

Comment 17

[Eyal Rozenberg]

Yes, I just can't, I'm swamped with course work. Which is why I apologized and
thanked Ginn Chen for the fix.