PresShell::CompleteMove simulates a mouse click and assumes LTRness

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
15 years ago
5 months ago

People

(Reporter: eyalroz, Assigned: eyalroz)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 3 obsolete attachments)

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).
Blocks: 262497
Summary: PresShell::CompleteMove mis-designed and assumes LTRness → PresShell::CompleteMove simulates a mouse click and assumes LTRness
Posted patch draft patch (obsolete) — Splinter Review
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.
Attachment #161408 - Flags: review?(daniel)
Posted patch patch v2 (obsolete) — Splinter Review
This version fixes the obvious bugs of the draft patch. Only tested it with
Thunderbird, though (i.e. not with caret browsing).
Assignee: events → eyalroz
Attachment #161408 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #161408 - Flags: review?(daniel)
Attachment #161521 - Flags: review?(daniel)
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.
Attachment #161521 - Flags: review?(daniel) → review?(roc)
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
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 on attachment 161521 [details] [diff] [review]
patch v2

let's see a new patch :-)
Attachment #161521 - Flags: review?(roc) → review-
Posted patch patch v3 (obsolete) — Splinter Review
- expanded the special-case to all br tags
- made changes as per roc's suggestion
- no real code in common with nsHTMLContentSerializer.cpp
Attachment #161521 - Attachment is obsolete: true
Attachment #162514 - Flags: review?(roc)
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.
Should the code of GetExtremeCaretPosition notice when elements have
style="visibility: hidden" ?
Posted patch patch v4Splinter Review
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
Attachment #162514 - Attachment is obsolete: true
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
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.
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.
Blocks: 278197
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.
Yes, I just can't, I'm swamped with course work. Which is why I apologized and
thanked Ginn Chen for the fix.
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.