Closed Bug 4302 Opened 25 years ago Closed 20 years ago

PgUp/PgDn in editor don't move caret/cursor (platform differences)

Categories

(Core :: DOM: Editor, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.2beta

People

(Reporter: jfriend, Assigned: mjudge)

References

Details

(Keywords: access, helpwanted, platform-parity, Whiteboard: [keybnd] EDITORBASE+ [QAHP] [ADT3 RTM] [ETA 06/04] [KEYBASE+])

Attachments

(4 files, 15 obsolete files)

1.55 KB, patch
akkzilla
: review+
Details | Diff | Splinter Review
1.02 KB, patch
Brade
: review+
Details | Diff | Splinter Review
3.15 KB, patch
akkzilla
: review+
Details | Diff | Splinter Review
104.63 KB, patch
mjudge
: review+
Details | Diff | Splinter Review
(This bug imported from BugSplat, Netscape's internal bugsystem.  It
was known there as bug #53588
http://scopus.netscape.com/bugsplat/show_bug.cgi?id=53588
Imported into Bugzilla on 03/26/99 10:35)

From the champs and it annoys me too.

Using PgUp and PgDn keys while composing an HTML message scrolls the
window, but doesn't move the cursor. The cursor remains where it was.  Keyboard
navigation in the editor is rendered fairly useless because you hit PgDn several
times, then hit the down arrow and it takes you right back up to one line below
where you were originally.
Sorry, this was one of the features that was cut from 4.0.
*** Bug 56040 has been marked as a duplicate of this bug. ***
Remind as per 4/10 bug triage.
it was a nice feature.  a very handy one if you had a long doc and needed to
scroll, only to find that once you type after scrolling, you're back to where
you started.
reopen this bug for 5.0
This should be working correctly on all Platforms in 5.0:
  Windows--caret moves
  Unix--caret moves
  Macintosh caret DOES NOT move
up priority to P1; this feature was completed for 5.0 but Mariner appears to
have broken it.
QA Contact: 4079
Status: REOPENED → ASSIGNED
ok ok
Summary: PgUp/PgDn in editor don't move cursor (platform differences) → (feature)PgUp/PgDn in editor don't move cursor (platform differences)
Target Milestone: M10
set milestone M10
Blocks: 10770
Summary: (feature)PgUp/PgDn in editor don't move cursor (platform differences) → (feature)PgUp/PgDn in editor don't move caret (platform differences)
fix summary
Target Milestone: M10 → M12
moving to m12
Whiteboard: post dogfood
Target Milestone: M12 → M15
changing milestone to after dogfood, setting to M15
Summary: (feature)PgUp/PgDn in editor don't move caret (platform differences) → [feature] PgUp/PgDn in editor don't move caret (platform differences)
Whiteboard: post dogfood → [beta f.c.]
feature clean-up work for beta
Summary: [feature] PgUp/PgDn in editor don't move caret (platform differences) → [BETA][feature] PgUp/PgDn in editor don't move caret (platform differences)
Whiteboard: [beta f.c.]
Target Milestone: M15 → M14
moving this up to m14
setting keyword
Keywords: beta1
Summary: [BETA][feature] PgUp/PgDn in editor don't move caret (platform differences) → [feature] PgUp/PgDn in editor don't move caret (platform differences)
Putting on PDT- radar for beta1.  Would not hold beta for this.
Whiteboard: [PDT-]
move to M15; remove beta1 keyword since this has PDT- designation
Keywords: beta1
Target Milestone: M14 → M15
M16 for now
OS: Windows NT → All
Whiteboard: [PDT-]
Target Milestone: M15 → M16
updating keyword and status whiteboard to reflect that this is a beta 2 feature 
work bug that the Composer team deems a must fix for beta 2.
Severity: normal → major
Keywords: beta2
Whiteboard: Composer feature work
Keywords: nsbeta2
[nsbeta2+ until 5/16]
Whiteboard: Composer feature work → [nsbeta2+ until 5/16] Composer feature work
This is bad...beppe filled in leger...get scoop from her.
Putting on [nsbeta2-] radar. Removing [5/16]. Missed the Netscape 6 feature 
train.  Please set to MFuture
Whiteboard: [nsbeta2+ until 5/16] Composer feature work → [nsbeta2-] Composer feature work
Keywords: nsbeta2
Whiteboard: [nsbeta2-] Composer feature work → [nsbeta2-]
Target Milestone: M16 → Future
*** Bug 29082 has been marked as a duplicate of this bug. ***
Whiteboard: [nsbeta2-]
adding help wanted to the keywords
Keywords: helpwanted
*** Bug 60172 has been marked as a duplicate of this bug. ***
Blocks: 58332
Keywords: nsbeta1
*** Bug 64547 has been marked as a duplicate of this bug. ***
*** Bug 58289 has been marked as a duplicate of this bug. ***
Keywords: access, mozilla0.9
Target Milestone: Future → ---
clearing milestone and also nominating for mozilla0.9.
Here are some comments from Mike about this bug:

I think the idea is:
 - to simulate down arrows until you get to the estimated location on the page 
and also trap for ending up on same line to avoid infinite loops and then 
 - to scroll the page so the caret shows up in approx the same spot relative to 
the container so the user doesn't have to look for the caret each time you hit 
page down

The "estimated" location is the tough one--you need the visual page size 
(assuming we are in a scroll view).  Find the containing scroll view  [from 
presshell]
Assignee: mjudge → brade
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla1.0
*** Bug 73581 has been marked as a duplicate of this bug. ***
Page Up and Page Down *must not* move the caret on Mac OS. It's in italics in 
the HIGs.
Brade, this behavior is written in the Apple Human Interface guidelines and is
the way Mac OS applications are supposed to work. The pgUp, pdDn, home, end, etc
keys are supposed to change the window scroll, not the focus. The guidelines
allow the arrow keys to change focus but not the pgUp, etc keys. Every Mac app I
use works this way. You can change it for other platforms, but please don't
change it on the Mac.
How do Mac users move the insertion point around if pgup, pgdn, home, and end 
only scroll?
They use a rodent :-( HIG wasn't designed for people with multiple fingers.

corvus@mac.com please post a HIG url if you intend for someone who you believe 
isn't well versed in HIG to do something according to HIG.
Keywords: mozilla0.9
The HIG note re. extended keys is at
http://devworld.apple.com/techpubs/mac/HIGuidelines/HIGuidelines-214.html#HEADING214-0

I also notice the arrow keys are not allowed to move out of multi-line text
fields, but the Tab key is.

I am one Mac user whose "pgDn pgDn pgDn, then press the up and down arrow to go
back to where I started" reflex is so ingrained that if the focus changed when I
pressed pgDn I would go insane.

I just tried this in 0.9 and the pgUp and pgDn keys didn't work at all. What gives?
Since I've got this bug, you can be sure that the Mac will follow HIG and not 
move the caret.  :-)  Mac users have other keybindings for moving the caret if 
desired (such as command-up arrow moving caret to the top of the window).
Keywords: pp
*** Bug 81596 has been marked as a duplicate of this bug. ***
*** Bug 84196 has been marked as a duplicate of this bug. ***
Priority: P1 → P2
Summary: [feature] PgUp/PgDn in editor don't move caret (platform differences) → PgUp/PgDn in editor don't move caret (platform differences)
Whiteboard: [keybnd]
f

























rr
*** Bug 86590 has been marked as a duplicate of this bug. ***
move to 0.9.3
reassign to mjudge

Mike--what needs to be done here is implementing:
 * nsPresShell::PageMove method 
(http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/
nsPresShell.cpp#3058) and
 * nsTextInputSelectionImpl::PageMove method 
(http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/
nsGfxTextControlFrame2.cpp#871)
Assignee: brade → mjudge
Target Milestone: mozilla1.0 → mozilla0.9.3
manager reviewed the need for the fix and agrees, approval for checkin to the
trunk and branch after fix has received an r= and sr=, adding nsBranch keyword
Keywords: nsBranch
*** Bug 89683 has been marked as a duplicate of this bug. ***
*** Bug 90637 has been marked as a duplicate of this bug. ***
This is really annoying, both html and non html mails
Catfood to me
Mostfreq added at 10 dups.
Keywords: mostfreq
*** Bug 91100 has been marked as a duplicate of this bug. ***
*** Bug 87170 has been marked as a duplicate of this bug. ***
Note: shift + pageup/down fails to work as one would assume also :)
missed 0.9.3.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
*** Bug 93793 has been marked as a duplicate of this bug. ***
if Mike can't get this resolved this coming week, this will get reassigned to
anthony
Whiteboard: [keybnd] → [keybnd] [nsBranch+]
-->anthonyd
Assignee: mjudge → anthonyd
Target Milestone: mozilla0.9.4 → mozilla0.9.5
*** Bug 96299 has been marked as a duplicate of this bug. ***
*** Bug 96816 has been marked as a duplicate of this bug. ***
*** Bug 97237 has been marked as a duplicate of this bug. ***
-->kin
Assignee: anthonyd → kin
--> mjudge owns selection and selection/caret navigation
Assignee: kin → mjudge
*** Bug 97906 has been marked as a duplicate of this bug. ***
If this is important to someone for 0.9.5, tell me now so I can find another
owner for it. mjudge won't be back until 0.9.6.
I think this bug is frustrating and should have been fixed waaaaaaaaaay back
(notice this bug was introduced in 1997!)
It's important (and frustrating) to me. With 17 dupes, 31 cc's, and 10 votes, 
it looks important to many others as well. Textareas need to move the cursor 
properly (windows/unix) with page up and down.
*** Bug 98991 has been marked as a duplicate of this bug. ***
Whow.. I've never seen a bug this old.. how about fixing it?
MozillaQu*st would just kick on this bug.. moving to M10.. to M12, "missed 0.9.3"...
Sorry for the sarcasm, Mozilla is great.
Blocks: 99194
removing nsbranch stuff...  
Keywords: nsbranch
Whiteboard: [keybnd] [nsBranch+] → [keybnd]
IMO, any platform-specific UI behavior should be a preference.  If Windows
users like the Mac behaviour, let them use Mac behaviour, and vice versa.
Also, some desktop environments for UNIX® systems (especially NeXT-derived
systems) try to adhere to the Mac behaviours.  Would it be too hard to make
"PgUp and PgDn move the caret" a preference that defaults one way on Mac
builds and the other way on Windows and UNIX builds?
*** Bug 100741 has been marked as a duplicate of this bug. ***
*** Bug 100876 has been marked as a duplicate of this bug. ***
*** Bug 101138 has been marked as a duplicate of this bug. ***
-> 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Blocks: 104166
Let's see...  P2, Major, 0.9.6, 15 votes, 38 CCs, and 22 dups.  nsbeta1 again? 
Maybe even nsCatFood?  Sorry for the spam, but this bug is four and a half years
old.
*** Bug 105070 has been marked as a duplicate of this bug. ***
This bug is four and a half year old, and that is the problem.
I think the next bug to fix is this one, so do it please.
This bug is actually *much* older than 4 1/2 years.  But that is irrelevant.

*IF* this bug is important to you, you should do more than complain in this bug.  
In fact, I don't think the person who is going to fix this bug has read the 
whining in this bug (and I can't blame him).  If you want this fixed, why don't 
you try to help?  Here are some of my ideas:

What does not help get this bug fixed:
 * Making bugzilla comments which add no technical insight
 * Encouraging others to vote for the bug (esp. if the bug is very old)
 * Sending e-mail to the bug owner and demanding to know when it will be fixed
 * Sending e-mail to the bug owner and complaining about it being broken

How to help?
 * Look at the code; produce a patch
 * Look for duplicate bugs in bugzilla
 * Triage other bugs on the bug owner's plate (look for dupes, invalids, etc.)
 * Produce test cases for other higher priority bugs this bug owner has
 * Produce patches for other higher prioirity bugs this bug owner has
 * Privately ask the bug owner how to help him get to this bug
     - offer to review patches
     - offer to test patches
     - offer to do testing on recently fixed bugs (make sure they don't regress)
 * Find someone else who can fix this bug and have them submit a patch
*** Bug 105401 has been marked as a duplicate of this bug. ***
This Probrem is included in all component.
the position of cursor doesn't change  while push PageUp and PageDown.
and this bad influence prevent you from selecting multi pages
with push Shift and PgUP/Down.

In the worse, if you push right in tail of text of Form "TEXTAREA" with vertical
scroll bar , cursor go top of text.

Confirmed with 2001101909&2001102203/Win2k.

http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=1521
*** Bug 100741 has been marked as a duplicate of this bug. ***
Yuki: part of your comment is covered by bug 82151, "Right arrow key at end of 
a TEXTAREA goes to the beginning".
*** Bug 106232 has been marked as a duplicate of this bug. ***
*** Bug 107361 has been marked as a duplicate of this bug. ***
*** Bug 108158 has been marked as a duplicate of this bug. ***
Whiteboard: [keybnd] → [keybnd] EDITORBASE [QAHP]
Marking nsbeta1+ per syd. Need to fix for composer usability.
Keywords: nsbeta1+
I searched on the single word "cursor" trying to find out if this bug existed 
and got "zaaro boogs", leading me to file yet another duplicate at bug 109935. 
I doubt most users associate the word "caret" with cursor schizophrenia.
Summary: PgUp/PgDn in editor don't move caret (platform differences) → PgUp/PgDn in editor don't move caret/cursor (platform differences)
0.9.6 is gone -> 0.9.7 
Target Milestone: mozilla0.9.6 → mozilla0.9.7
*** Bug 111755 has been marked as a duplicate of this bug. ***
*** Bug 109935 has been marked as a duplicate of this bug. ***
*** Bug 112620 has been marked as a duplicate of this bug. ***
*** Bug 113065 has been marked as a duplicate of this bug. ***
Confirm comment #76: As point of clarification, this errant behavior also
applies when filling in forms (e.g. http://www.netseller.com/tecfrm.htm) and
composing text email.
might as well move it to 0.9.8 now.. :)
*** Bug 115653 has been marked as a duplicate of this bug. ***
*** Bug 117125 has been marked as a duplicate of this bug. ***
*** Bug 117755 has been marked as a duplicate of this bug. ***
*** Bug 120741 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Blocks: 100741
*** Bug 118799 has been marked as a duplicate of this bug. ***
Mozilla 0.9.8 is nearly out the door, and I don't see a patch yet.
Suggest 0.9.9?
marking editorbase plus
Whiteboard: [keybnd] EDITORBASE [QAHP] → [keybnd] EDITORBASE+ [QAHP]
moving to 1.0
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.8 → mozilla1.0
*** Bug 123773 has been marked as a duplicate of this bug. ***
Blocks: advocacybugs
*** Bug 128231 has been marked as a duplicate of this bug. ***
*** Bug 128036 has been marked as a duplicate of this bug. ***
removing myself from the cc list
*** Bug 131028 has been marked as a duplicate of this bug. ***
editorbase-, nsbeta1- per triage
Keywords: nsbeta1+nsbeta1-
Whiteboard: [keybnd] EDITORBASE+ [QAHP] → [keybnd] EDITORBASE- [QAHP]
Why is 4xp missing from keywords? This old thing should have been history long ago.
This makes the composer crap for me. I hate going for the mouse.
I've looked at the sources but I don't think that there could be a correct, easy
fix.
I can think of some hacks for example:

1. Get XY coordinates of the caret.
2. Is it possible to scroll by page (at least some amount)? 
3a. Yes. Scroll by page (this is done now).
4a. Put the caret on the same XY position as it was before.
3b. No. Put the caret on the beginning/end of the edited text.

This shouldn't be too hard to do IF it's possible to get the current XY coords
of the caret.
Can someone with the power please add keyword 4xp? Netscape 4 had no such
trouble. The presence of this bug is monumentally absurd.
4xp is not correct since Composer in 4.x (or any version prior) did not support
page up/down with caret movement.
OS/2 2002032416

Wrong. I use [about:bldlevel: Netscape Communicator 4.6 was created on 17 Dec
2001 at 21:18:23: Support for 128 bit encryption and mail encryption] every day.
PgUp/PgDn maintains cursor in the viewable area always. It works in 3.x. It
works in 4.x. No excuse for it to not work in Mozilla. Definitely 4xp.
s/maintains cursor in the viewable area always./maintains cursor in the viewable
area always while composing plain text email & news./

I've just checked 4.79 for windoze, and the cursor in maintained in the viewable
area always while composing plain text email & news and using the PgUp/PgDn keys.

I've just checked 4.77 for Caldera OpenLinux 3.1/KDE 2.1, and the PgUp/PgDn keys
don't even scroll the text email composition window, much less move the cursor
anywhere. (System not used much & prolly misconfigured).
I just tried 4.75 on linux (the version that comes with Redhat 7.1).  Page up
works, but does not move the caret.  So it's not 4xp on linux, though apparently
it was on Windows and OS/2.

Tom: this shouldn't require anything to do with XY coordinates of the caret. 
What needs to be done is to change the selection commands (in the selection
controller) so that (configurably, since it's different on different platforms)
the selection is collapsed to some element of the document that's visible in the
page after the scroll.  (The caret follows the collapsed selection.)  I'm not
sure how you find out which part of the document is currently visible, but that
might be evident from whatever code it's already using to scroll (I'm not
familiar with the scrolling code).
Slackware Linux 8
Netscape(r) Communicator 4.77 
Copyright (c) 1994-2001 Netscape Communications Corporation, All rights reserved. 

PgUp/Down work as expected: they page up or down exactly one screen also moving
caret.
Blocks: atfmeta
*** Bug 133450 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.0 → mozilla1.1alpha
*** Bug 137949 has been marked as a duplicate of this bug. ***
*** Bug 138073 has been marked as a duplicate of this bug. ***
How can 1.0 release possibly be justified without a fix for this? This is
ancient (over four years old), major, and 4xp. This, bug 118672, bug 118899, bug
137018, and bug 109935 amount to stoppers preventing upgrade from 4.x mailnews.
I can't count. s/four/five/. Sheesh.
Everyone knows this bug is very very old bug.
Do not complain more, but let's discuss the solution.

nsTextInputSelectionImpl::PageMove() do PgUp/PgDn action.
http://lxr.mozilla.org/seamonkey/source/layout/html/forms/src/nsGfxTextControlFrame2.cpp#915

In sources, nsTextInputSelectionImpl::PageMove() has codes
which is not used now. That may be what Tom said at comment 107.
That codes calculate the position of the caret after scroll
and intend to collapse on a frame at the position.
(Is this bad codes, Akkana?)
Unfortunately, the codes have not been completed.

I tried to complete that codes.
There is a difficulty in getting specified frame from the position (struct nsPoint).
I thought to use nsGfxTextControlFrame2::GetFrameForPoint().
But I could't call it within nsTextInputSelectionImpl::PageMove().
Because I couldn't refer to nsGfxTextControlFrame2 instances there.

Does anyone have good ideas?
What does GetPrimaryFrameFor() on mLimiter give you?
> (Is this bad codes, Akkana?)

I'm not familiar with this code at all.  Looks like brade wrote the current
partial implementation, though Mike is really the expert on how this stuff works.
*** Bug 130380 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.1alpha → mozilla1.0
Attached patch experimental patch for Textarea (obsolete) — Splinter Review
I was inspired by Boris's idea and wrote this patch.
This is not complete version, but works.

I want a specification about the behavior:
Where the caret should appear after PgUp/PgDn hitted?
At the Top of the area (Netscape4.77/UNIX does) or Bottom of the area?
Emacs is bottom for PgUp, top for PgDn.
This patch works as Emacs do.
Is there any specification?

And I want some help.
This patch uses nsPresShell::ScrollFrameIntoView().
But this method has a difficulty in scrolling horizontally
to make the caret into view area.
Any alternatives or ideas?
Could you possibly use scrollSelectionIntoView() ?  Does that work for a
collapsed selection (ie the caret)?
Cursor should fall in the same relative vertical position in the viewspace as it
was before the PgDn/PgUp keys, unless the end of space is reached. In that case,
PgUp should leave cursor at 0,0, and PgDn, at 0,last.
I suggest going with the emacs approach myself.... but then, I'm used to it.  :)
People who know little or nothing about *nix might like to know what the emacs
appoach is, like me.
> Cursor should fall in the same relative vertical position in the viewspace as it
> was before the PgDn/PgUp keys, unless the end of space is reached. In that case,
> PgUp should leave cursor at 0,0, and PgDn, at 0,last.

And it should also keep in the same horizontal offset.  For lines size >= the
prev line, h should remain unchanged.  Since Moz text buffer lines wrap to the
next on h pos > current line len, you should only trim into h pos of line len if
new line len is < old line len.

IE:  If you have a space of thousands of lines all of len 5 and your cursor is
on 4, pressing pgdown should never move it to 0 h pos.  If one of the lines we
land on while going down is len 3, we should then be at h pos 3.

Sound good?  This reflects the vast majority of text editors people use on
Windows, MacOS, and things like pico, cooledit, gnotepade, gedit, kde text
editor, and many other easy-to-learn editors (emacs or vi options are, imo,
useful to a much smaller audience and should only be available as a non-default
option).
Looks to me like an incomplete explanation.
Excellent editor behavior is one of several reasons why I continue to use
Netscape 3 (three, not 4) for most email.
> Looks to me like an incomplete explanation.

What is this in relation to?  If it's to comment 129, please understand that I'm
just specifiying the behaviour of the cursor's x position on a line when
pgup/down adjustments are applied to the location.  A complete description of
moving the cursor in all cases in a general editor would require several pages
of text, and would be redundant.

As for using NS3, no CSS support, no XHTML support -- no thanks :p
Attached patch Proposed Patch for Textarea v1 (obsolete) — Splinter Review
Thanks, Boris.
ScrollSelectionIntoView() works fine.
And I fixed the behavior as comment #125.
This patch still has Emacs-like behavior.
For me, this is rather satisfying one.

For wrap problem of comment #129,
we probably should know nsSelection::mDesiredX.
But, there is no way to know it because it is private.
Does anyone have some idea?
Attachment #81290 - Attachment is obsolete: true
I tried nsPresShell::PageMove() with same way.
This patch works for Composer and text/HTML Mail composer.
This makes a good job in most cases.
But some bugs are there.
(In some cases, caret will go away.
In some cases, it forgets horizontal position.)
Oops, I said wrong.
> And I fixed the behavior as comment #125.
read this as comment #129.

What I wanted to say here is that it trys to keep the horizontal offset.
Comment on attachment 81493 [details] [diff] [review]
Proposed Patch for Textarea v1

lets get this into the trunk. I would remove the ifdef MAC since the
keybindings will take care of calling or not calling this code.
Attachment #81493 - Flags: review+
Comment on attachment 81496 [details] [diff] [review]
experimental patch for nsPresShell

again aside from the IFDEF this looks good. lets give it a try on the trunk
Attachment #81496 - Flags: review+
Agreed about the ifdefs.  This desired behavior is the whole reason we have
separate methods for pageMove (which moves the caret) vs scrollPage (which
doesn't).  The key bindings should call the appropriate method, so there should
be no need for platform ifdefs in the C++ code.  

Currently it looks like both the mac and the global bindings call
cmd_scrollPageUp/Down, so one of them probably needs to change to call pageMove.
Attached patch Proposed Patch for Textarea v2 (obsolete) — Splinter Review
OK. I removed #ifdef.
Attachment #81493 - Attachment is obsolete: true
For the patch for nsPresShell, I removed #ifdef and debug codes.
This patch sometimes behave wrong as said above.
Is it ok?
Attachment #81496 - Attachment is obsolete: true
*** Bug 141804 has been marked as a duplicate of this bug. ***
I am testing the latest patches in my Mac build.
In Composer, I don't see it page down and move the caret always when I press
alt-down arrow.  Pressing the down arrow does move it down further.  Maybe it's
just some table oddity?  I'm testing by editing:  http://mozilla.org/editor/

Alt-up and alt-down don't seem to work in textareas; I'll investigate that
(probably a missing keybinding?)
OK, in both patches, I see the problem that shift doesn't work to change
selection; is that a known issue?  Can these patches be revised to handle that
or should we reopen one of the duplicate bugs to this bug which deals with shift
issue specifically?

I have a patch for the Mac keybindings which should be included with the other
patches.  I will attach shortly.
Wait, I'm confused about what we're doing here.  If we're going to check in a
patch, can we make sure we're fixing it right rather than making a bigger mess
for someone to clean up later?  It's great that we have a patch and we can
finally fix this, but I think we should take a little time to make sure it's the
right patch before checking in.

What do we have, and what do we want to have?

As I understand it, there are two sets of commands, cmd_movePageUp/Down vs.
cmd_scrollPageUp/Down.

Currently (someone please correct me if this is wrong):
XP bindings use "scroll" for editor, nothing for textareas.
Win platform bindings use move for textareas and for editor.
Mac platform bindings use scroll for textareas and for editor.
Linux platform bindings use move for textareas and for editor.
The implementations for both scroll and move do a scroll but do not move the caret.

What we should have (again, correct me if I have this wrong):
Implementation for move should move the caret, implementation for scroll should
not.  (No ifdefs needed, there are two separate routines.)
xp bindings: use move for textareas and editor.
mac platform bindings: use scroll for textareas and editor.
unix/win platform bindings: don't list a binding, default to xp bindings.
*** Bug 141913 has been marked as a duplicate of this bug. ***
Bug 58332 (listed as being blocked by this bug) is for the shifted version of
this functionality (using aExtend parameter in the current patches).

Akkana--You are correct about the various commands and their keybindings.  I
don't think the current patches have any platform ifdefs.  This bug covers the
core functionality for moving the caret in "page" chunks.  

I attached the Mac keybinding fix here since it should land with the fixes for
this bug.  I haven't checked any of the other keybindings yet; those may need
changes also.  Akkana--could you r= it?

Is everyone ok with this being checked in without dealing with the shift
(extend) variation working?
skamio--would it be possible for you to easily fix the extending issue too
(while you're already modifying the code)?
Bug 58332 (the shifted version) is marked future, but to me feels like the same 
useability issue and should be given the same priority. just my $0.02.
I can't but heartily agree with you. The shifted version (bug #58332) is in my
opinion tightly connected to this one (#4302) and both should be resolved at
once. If possible, please fix them both for 1.0.
Kathy and I went through this -- she'd prefer keeping the current overrides in
all of the platformHTMLBindings files, and remove the lines in the xp file,
rather than correcting the xp bindings to do move and removing the linux/win
platform overrides.  I'm willing to go with that.  Here's a patch which removes
the existing xp bindings -- they were to the wrong routine, they were only
there for editor and not textareas, and they were overridden on all three
platforms anyway.
Comment on attachment 82103 [details] [diff] [review]
patch to fix mac keybindings for textareas

r=akkana on Kathy's mac key bindings.
Attachment #82103 - Flags: review+
One comment on the current implementations:
I notice that pageMove acts like Windows scrolling, in other words: if the caret
is at the top of the page and I hit page down, it moves the caret to the bottom
but doesn't actually page down.

On Unix, my expectation is that hitting page down will actually page down (and
leave the caret either at the top of the screen or at approximately the same
position it was on the previous page).

I don't have any problem with this patch going in as is, but if it does, I'd
like to either file a new bug or keep this one open to make it possible to make
pageMove scroll instead of just moving the caret the first time.  (Post 1.0
would be fine for that -- it's not that important, unless there's a quick fix we
could put in now.)

Re extending selection: neither windows nor unix has bindings for extending the
selection with page up/down keys.  They probably should, when extending the
selection gets addressed.
Comment on attachment 82234 [details] [diff] [review]
Patch to remove the unused and incorrect xp bindings

r=brade
Attachment #82234 - Flags: review+
Time to mention relative vertical positions, I guess :)

Unless next page size is less than the size of the scroll, move the scrolled
text by that amount.  The cursor should be at the same relative y offset (IE: If
I have a 4 page document, and I'm on line 2 and hit page down, and the page
scroll amount is 25, my "new" caret absolute pos is 27 but relative is still 2).

In cases where the scroll amount is smaller than the page scroll, jump the caret
to the end/start of the text.  This behaviour is once again the most common and
(IMO) most logical handling of it, because you can lessen the amount of
mouse/arrow work to get to a specific position.

Plus everyone has used it ;)
Attached patch Proposed Patch for Textarea v3 (obsolete) — Splinter Review
I fixed the extending issue.

And I implementated the behavior of keeping relative position of the caret in
view.
(Netscape 4.77/UNIX does. I misunderstanded before :-)
Attachment #81816 - Attachment is obsolete: true
I fixed the extending issue, too.
And implemented the behavior keeping relative position of caret.

This patch forgets the horizontal offset in plaintext mail composer
when the caret hits top or bottom of the document.
Probably, this is because containerHeight has some margin.
So, the calculation in this patch get a wrong value.
Attachment #81817 - Attachment is obsolete: true
This patch fixes the keybinding for the extending issue.
If you try two patches above, use this, too.
Comment on attachment 82480 [details] [diff] [review]
Patch to fix SHIFT+PgDp/PgDn keybinding for PC and UNIX

r=akkana on the linux/windows bindings.

The new patches for textareas and pres shell work beautifully on linux, both
shifted and unshifted.	Thanks, Shotaro!  Kathy, have you had a chance to try
them on Mac?
Attachment #82480 - Flags: review+
Does the proposed patch fix this problem for caret browsing as well? It should.

What did you mean by "this problem for caret browsing", dmitry?
I implemented the caret behavior mentioned in comment #152 and comment #154.
Or you meant other problem?
What he means is the following: if you hit "F7" that will toggle on
"browse-with-caret" mode, which puts a visible caret (similar to the one in
composer) in the browser's content area.  The question is whether
pagedown/pageup move this caret with the patches in this bug.  I suspect the
nsPresShell changes handle that case...
Yes, what I was referring to is the f7 "caret browsing." It also has another
related problem which I filed today as bug 144000.
Oh, I see. (I hadn't known "caret-browsing" until now...)

I tested and my patch moved the visible caret when I hit PgUp/PgDn.
I think you love it :-)
*** Bug 144324 has been marked as a duplicate of this bug. ***
Attached patch new pres shell patch (obsolete) — Splinter Review
Talked to kin to sr the old patch and he has some reservations about the size
and reuse.  i factored out some other parts of presshell to trim down
nsPresShell patch. also we needed to factor in the SCROLL_PERCENTAGE into the
scroll. when you scroll up or down you actually only move 90%.	Also it is ok
to let the number go out of bounds and let getcontentoffsets bind it to the
view. no need to look for out of bounds there.	The next patch is required to
move SCROLL_PERCENTAGE to a place outside people can see.  I will see if i can
factor this code more to allow text views to also reuse the same code.
Attachment #82478 - Attachment is obsolete: true
see above comment for why we need this patch as well.
*** Bug 144591 has been marked as a duplicate of this bug. ***
this reuses most code by passing arguments to CommonPageMove in nsPresShell.
small change to nsIPresShell to contain this method but its for a good cause of
code reuse. scroll view code change is just to move a #define.	( i am open to
a future bug about using an enum or something else..).	Basically the same as
original patch with some extra stuff taken out and some harsh reuse to apease
the great kin ;)
Attachment #82475 - Attachment is obsolete: true
Attachment #83599 - Attachment is obsolete: true
Attachment #83601 - Attachment is obsolete: true
*** Bug 145983 has been marked as a duplicate of this bug. ***
Keywords: nsbeta1-nsbeta1+
Whiteboard: [keybnd] EDITORBASE- [QAHP] → [keybnd] EDITORBASE- [QAHP] [ADT3 RTM] [ETA 06/04]
*** Bug 146713 has been marked as a duplicate of this bug. ***
*** Bug 147379 has been marked as a duplicate of this bug. ***
Blocks: 143047
It turns out that my problem starting mozilla was not a problem in that day's
build; it's a problem with the patch.  When I apply the patch to a working
build, then build in xpfe and layout, mozilla -edit no longer brings up a
window.  It gets through all the usual registering of libraries and nsKeyboard
complaints and other warnings, then simply exits with status 11 without ever
showing a window or printing an abnormal error message.  

The last few lines on stdout are:

LoadPlugin()
/builds/vanilla/mozilla/modules/plugin/samples/default/unix/libnullplugin.so
returned 81ee090
GetMIMEDescription() returned "*:.*:All types"
WEBSHELL+ = 2
bad FastLoad file version
warning: property switchskins already exists
warning: property switchskinstitle already exists
Note: verifyreflow is disabled
Note: styleverifytree is disabled
warning: property cp1256 already exists
Note: frameverifytree is disabled
WARNING: 

charset = ISO, file nsFontMetricsGTK.cpp, line 1995
hmm let me clarify this.  akkana had not built her whole tree so the vtable 
offsets were wrong for some of her dlls so the crash would happen.  after she 
rebuilt the whole tree she was able to run the build.  I am still waiting on 
her comments for the actual patch now that her tree seems to be wurkin.
So what's the current status of the patch? Can somebody r= and sr= it?
Comment on attachment 83688 [details] [diff] [review]
total patch for nsPresShell, gfxtextframe and the small view change

>+nsIFrame *
>+PresShell::GetPrimaryFrameFromTag(const nsAString& aTagname)

Don't we already have methods to get the document root?  For instance, 
I see PresShell::GetRootFrame -- does that get something different from the
frame for the body node?  And nsIDocument has GetRootContent.  Seems like it
might be better to use one of these rather than reinventing the wheel.

>+    if (NS_COMFALSE == result) //NS_COMFALSE should ALSO break
>+      break;
>+    if (NS_OK != result || !pos.mResultFrame ) 
>+      return result?result:NS_ERROR_FAILURE;

NS_COMFALSE is deprecated, according to the comment in nsError.h.  I know you 
didn't introduce it in this patch, but can we fix it as long as we're here?
Or is this really what you want to do?	If so, please add a comment explaining
it so people encountering the obsolete NS_COMFALSE will know why it's there.

>Index: view/public/nsIScrollableView.h
>===================================================================
>+// the percentage of the page that is scrolled on a page up or down
>+#define PAGE_SCROLL_PERCENT 0.9

If this has to be in an interface file, might it be better to make it an enum?

Otherwise, looks okay and seems to work as expected.
I Didn't get all the way through the diff yet, hopefully I can get some time to
complete it next week. Here are my comments so far:


--- This comment is very hard to read/understand:


+  /**
+   * Scrolling then moving caret placement code in common to text areas and 
+   * content areas should be located in the implementer
+   * This method will accept the following parameters and perform the scroll 
+   * and caret movement.  It remains for the caller to call the final 
+   * ScrollCaretIntoView if that called wants to be sure the caret is always
+   * visible.


--- The code in CommonPageMove() seems to assume that the caret's closest view
    and the view for the document are the same. I don't think that's a correct
    assumption since that could throw off the coordinates used in finding where
    the caret should be placed after the scroll.


--- PageMove() calls GetPrimaryFrameFromTag() with "body", does that imply that
    this feature isn't supposed to work with XML documents. Remember that there
    is "Browse With Caret" feature in the browser for accessibility that turns
    on the caret.
bumping off to 1.2
Target Milestone: mozilla1.0 → mozilla1.2beta
Blocks: 151481
*** Bug 153634 has been marked as a duplicate of this bug. ***
Mike, where does this bug stand?
Whiteboard: [keybnd] EDITORBASE- [QAHP] [ADT3 RTM] [ETA 06/04] → [keybnd] EDITORBASE- [QAHP] [ADT3 RTM] [ETA 06/04] [KEYBASE+]
Comment on attachment 83688 [details] [diff] [review]
total patch for nsPresShell, gfxtextframe and the small view change

Ok, here's the rest of my comments. I think the patch still needs some work:

--- Correct the "bahavior" typo:


-#if XXXXX_WORK_TO_BE_COMPLETED_XXXXX
-  // XXX: this code needs to be finished or rewritten
   // expected bahavior for PageMove is to scroll AND move the caret
-  nsIScrollableView *scrollableView;


--- It bugs me that we have to know the frame structure to get to
    the Div's area frame in the nsGfxTextControlFrame2.cpp code.
    Can't we just call GetScrolledView() on the scrollable view
    and call GetClientData() on it which might actually return the
    frame we are interested in? If that really does work, then we
    can avoid having to know about "body" and "div", and the fact
     that the doc might not even be HTML in both the presShell code
    and the nsGfxTextControlFrame2 code. I'm also worried about the
    perf impact of calling doc->GetElementsByTagName() ... does it
    crawl the entire content tree looking for all elements that
    could be called "body" or is it optimized to know there is only
    one body?

    Here's the code I was referring to that knows the frame hiearchy
    for the div:


+    //get the scroll port frame from the gfxscroll frame
+    if (NS_FAILED(result = frame->FirstChild(context, nsnull, &frame)))
+      return result;
+    if (!frame)
+      return NS_ERROR_UNEXPECTED;
+    //get the area frame (what we really want) from the scroll port.
+    if (NS_FAILED(result = frame->FirstChild(context, nsnull, &frame)))
+      return result;
+    if (!frame)
+      return NS_ERROR_UNEXPECTED;


--- By the way, in my previous review comments I mentioned that the
    caret could be in a frame that is in a view that is not the same
    as the scrolled view. An example of this could be a div with
    an overflow property that was specified, for example:


      <div style="overflow: none">a<br>b<br>c<br>d<br>e<br>f<br></div>
Attachment #83688 - Flags: needs-work+
> does it crawl the entire content tree looking for all elements that could be
> called "body"

At the moment, yes.  In a few days it will no longer do so (I have a patch that
fixes that and it should land soon).  _However_ the code does GetLength() on the
list, and at that point we will have to crawl the whole document.  We should
remove the GetLength() check and just null-check the node Item() returns -- if
it's null, we return nsnull.
*** Bug 154868 has been marked as a duplicate of this bug. ***
*** Bug 155810 has been marked as a duplicate of this bug. ***
Blocks: 156258
*** Bug 156469 has been marked as a duplicate of this bug. ***
How will the fix interfere with browser search? At least, bug 156519 will become
more important ...
Putting this back on the EDITORBASE+ list based on user feedback. This needs to
get fixed.
Whiteboard: [keybnd] EDITORBASE- [QAHP] [ADT3 RTM] [ETA 06/04] [KEYBASE+] → [keybnd] EDITORBASE+ [QAHP] [ADT3 RTM] [ETA 06/04] [KEYBASE+]
when this is fixed, please try scenario in dup bug 147379.
ok so i removed the getlength call on the node list and instead just get the 
first element on the list and check it for null.  I also replaced the 
predetermined frame structure assumptions and replaced them with this:
--
    nsIView *scrolledView;
    result = scrollableView->GetScrolledView(scrolledView);
    void*    clientData;
    nsIFrame* frame = nsnull;
	
    // The view's client data points back to its frame
    if (scrolledView && NS_SUCCEEDED(scrolledView->GetClientData(clientData))) {
      nsISupports* data = (nsISupports*)clientData;
  
      if (nsnull != data) {
        if (NS_FAILED(data->QueryInterface(NS_GET_IID(nsIFrame), (void 
**&frame))) {
          frame = nsnull;
        }
      }
    }


So far so good.
Attached patch new patch with fixes (obsolete) — Splinter Review
this should fix the comments kin had
spiff, are we ready for r/sr?
at a glance, in PresShell::CommonPageMove, it looks like beginFrameContent is
not initialized; is that a problem?
Comment on attachment 92604 [details] [diff] [review]
new patch with fixes

The issues in comment 176 still need to be addressed. And the typo mentioned in
comment 180 is actually in 2 places in this new patch.
Attachment #92604 - Flags: needs-work+
Attached patch big patch with all in it. (obsolete) — Splinter Review
ignore nsHRFrame and nsFrame.cpp in diff. those are for bug 159207.
Attachment #83688 - Attachment is obsolete: true
Attachment #92604 - Attachment is obsolete: true
Comment on attachment 93439 [details] [diff] [review]
big patch with all in it. 

=============
= nsCaret.cpp
=============

-- In GetCaretCoordinates() you need to remove the setting of outView
immediately
   after the call to GetViewForRendering() or it will overwrite the value
   returned by GetViewForRendering():

      -  GetViewForRendering(theFrame, aRelativeToType, viewOffset, clipRect,
drawingView);
      +  GetViewForRendering(theFrame, aRelativeToType, viewOffset, clipRect,
&drawingView, outView);
	 if (!drawingView)
	   return NS_ERROR_UNEXPECTED;
      -
      +  if (outView)
      +    *outView = drawingView;
	 // ramp up to make a rendering context for measuring text.
	 // First, we get the pres context ...

-- You'll get brownie points if you clean up the arg naming for
   GetCaretCoordinates(). :-)

================
= nsIPresShell.h
================

-- I just noticed that you're adding CommonPageMove() to nsIPresShell.h. It
   should go in nsISelectionController.idl with the other navigaion methods
   shouldn't it?


======================
= nsGfxScrollFrame.cpp
======================

-- In nsGfxScrollFrame.cpp you declare and initialize point:

      +  nsPoint point;
      +  nsPoint currentPoint;
      +  point.x = 0;
      +  point.y = 0;
      +  //we need to translate the coordinates to the inner


   several lines above it's first use, which just resets the value:

      +  point = aPoint;
      +  while (view != innerView && innerView)

   So why not just delay the declaration till the point they are actually used
   and just initialize it like this:

      +  nsPoint point(aPoint);
      +  nsPoint currentPoint;
      +
      +  while (view != innerView && innerView)


-- I know I was sitting with you when you made this change:

      +  result = GetClosestViewForFrame(aCX, frame, &innerView);
      +  if (NS_FAILED(result))
      +    innerView = nsnull;

   I'm thinking now that we should just return an error in the failed case
   to be consistent with the GetClosestViewForFrame() call before this one.


-- You can remove this:

      +
      +#if 1
      +#endif


=================
= nsPresShell.cpp
=================

-- In nsPresShell.cpp, CommonPageMove() in the class declaration should be
   grouped with the other nsISelectionController method declarations.

      +  NS_IMETHOD CommonPageMove(PRBool aForward, PRBool aExtend,
nsIScrollableView *aScrollableView, nsIFrame *aMainFrame,nsIFrameSelection
*aFrameSel);

-- As mentioned in prior review comments, you need to get rid of
   GetPrimaryFrameFromTag() and all it's uses in your new code. Relying on it
to
   give you back the frame for "body" means the code that uses it won't work
for
   XML pages. Replace it with a utility method like
   GetTopLevelScrolledViewFrame() which returns the clientdata of the top-level
   scrolled view ... similar to what you do in nsGfxTextControlFrame2.cpp
diffs.

-- In CommonPageMove() don't be afraid to use vertical white space! :-)

-- "bahavior" is still misspelled.

      +    return NS_ERROR_NULL_POINTER;
      +  // expected bahavior for PageMove is to scroll AND move the caret
      +  // and remain relative position of the caret in view. see Bug 4302.

-- Checking for failure is not necessary here since you are returning anyways:

      +  result = aFrameSel->HandleClick(content, startOffset, startOffset,
aExtend, PR_FALSE, PR_TRUE);
      +  if (NS_FAILED(result)) 
      +    return result;
	 return result;

-- PageMove() already gets the scrollable view, it should just get the frame
   from it's scrolled view instead of calling:

      +  nsIFrame *frame = GetPrimaryFrameFromTag(NS_LITERAL_STRING("body"));

-- CompleteMove() should also use the root scrolled view's frame instead of:

      +  nsIFrame *frame = GetPrimaryFrameFromTag(NS_LITERAL_STRING("body"));


============================
= nsGfxTextControlFrame2.cpp
============================

-- "bahavior" is still misspelled:

      -#if XXXXX_WORK_TO_BE_COMPLETED_XXXXX
      -  // XXX: this code needs to be finished or rewritten
	 // expected bahavior for PageMove is to scroll AND move the caret
      -  nsIScrollableView *scrollableView;

-- I think the word "to" is missing in this comment:

      +  // and remain relative position of the caret in view. see Bug 4302.
Attachment #93439 - Flags: needs-work+
Attached patch big patch part deu (obsolete) — Splinter Review
ok besides nsFrame and nsHRFrame which should be ignored in this patch, I think
this is the mother of all patches.  this should address all the issues above.
bahavior isnt in the tree at all as far as I can tell.	I moved CommonPageMove
to nsSelection and removed it from nsIPresShell.  I have added some whitespace
to CommonPageMove in my local tree that is not in this patch so you will just
have to take my word for it.
Attachment #93439 - Attachment is obsolete: true
mjudge:
   // expected bahavior for PageMove is to scroll AND move the caret
from your patch. you didn't touch that line, but you can fix it while you're at it.
it's in layout/html/forms/src/nsGfxTextControlFrame2.cpp near line 872
yes, please fix the typo/misspelling for "bahavior" in that comment.  :-)
son of a.  i dont understand its in my tree. maybe i posted the wrong patch. I 
will run it again. trust me I ran a search of all files for this misspelling.
Attached patch patch to fix spelling error. (obsolete) — Splinter Review
Attachment #93571 - Attachment is obsolete: true
I am pulling new trees to split up my bugs.  the last patch has 2 other bugs in 
it and its getting confusing.  This will probably take until friday to get done.
last patch had included some other bugs along with it.	I have made a new tree
with just this bug included. I believe that this will allow more easy reviews
by the reviewers.
Attachment #93620 - Attachment is obsolete: true
needs a review from someone....
> needs a review from someon

What kind of review are you looking for ?

Ajay
*** Bug 161988 has been marked as a duplicate of this bug. ***
patch will no longer work i guess. it needs to be updated. I will try to post
yet another patch.
Comment on attachment 93767 [details] [diff] [review]
total patch without other bugs included

r = jfrancis;
Tried to concentrate on big picture rather than line by line stuff...

nsSelection.cpp ================================================

CommonPageMove(): null check for aFrameSel I see that we dont
care if selection is collapsed or not... assuming thats ok.
ScrollByPages() does not scroll the PAGE_SCROLL_PERCENT, it
scrolls a whole page. So the HandleClick() call may be on content
that is not scrolled into view.  What happens then?


nsCaret.cpp ====================================================

This looks ok though I dont pretend to understand the view stuff


nsGfxScrollFrame.cpp ===========================================

"while (view != innerView && innerView)" - oh sure, make me look
up operator precendence you big wanker. btw, I didnt even realize
this source file was stil part of build.

nsPresShell.cpp ================================================

PageMove(): I see this calls ScrollSelectionIntoView() after
CommonPageMove().  I'm worried that the PAGE_SCROLL_PERCENT
dealio is going to make us rely on ScrollSelectionIntoView() for
some page downs but not for others, depending on how close to the
top(bottom) of the page the caret was. will this lead to
different behaviors, like getting the view scrolled with the
caret centered sometimes, for example?


nsGfxScrollFrame2.cpp ==========================================

Looks good, though same concerns apply as to when
ScrollSelectionIntoView is needed.


nsTextControlFrame.cpp =========================================

Ditto.	By the way, is there any way to share common code between
nsGfxScrollFrame2.cpp & nsGfxScrollFrame2.cpp?
Attachment #93767 - Flags: review+
uhh, last line shoulda been:
"nsGfxScrollFrame2.cpp & nsTextControlFrame.cpp?"
ok, so I meant nsGfxTextControlFrame, instead of nsGfxScrollFrame.  What's in a
name, anyway?  

I spoke with Kin about my concerns.  He pointed out that I was looking at the
wrong ScrollByPages() function, and the correct one does use the page scaling
constant.  So no worries there.  And the two sets of changes I asked to be made
common are in fact in the same file, which aparently was renamed.

So I don't have any issues with this patch other than the perhaps unneeded call
to ScrollContentIntoView(), and a missing null check for aFrameSel.
posting new patch with updated tree in it. carrying over the r=
new patch with new make system minus nsGfxTextControlFrame.  Havent put in
jfrancis's mods yet.  I will do so today.  I wont post new patch with changes
in them unless requested.
Attachment #93767 - Attachment is obsolete: true
Comment on attachment 95309 [details] [diff] [review]
patch from /mozilla

carrying over the r=
Attachment #95309 - Flags: review+
Comment on attachment 95309 [details] [diff] [review]
patch from /mozilla

-- I still have trouble parsing this first sentence in this comment, is it
   possible to reword it?


+  /**
+   * Scrolling then moving caret placement code in common to text areas and
+   * content areas should be located in the implementer


-- For CommonPageMove(), there seems to be an implicit assumption that
   aMainFrame *is* the frame associated with the scrolled view inside
   aScrollableView.

+   * @param aScrollableView the view that needs the scrolling
+   *
+   * @param aMainFrame the contained parent frame inside the scrollable view.
+   *

If you moved the code that fetched the frame into CommonPageMove() then you
could remove the code that does the same thing in PresShell::PageMove() and
nsTextInputSelectionImpl::PageMove(). Also, why the 2 different methods of
transforming clientData into an nsIFrame? One does a straight cast:


+  nsIView *scrolledView;
+  result = scrollableView->GetScrolledView(scrolledView);
+  // get a frame
+  void *clientData;
+  scrolledView->GetClientData(clientData);
+  nsIFrame *frame = (nsIFrame *)clientData;


  and the other uses QI:


+    nsIView *scrolledView;
+    result = scrollableView->GetScrolledView(scrolledView);
+    void*    clientData;
+    nsIFrame* frame = nsnull;
+
+    // The view's client data points back to its frame
+    if (scrolledView && NS_SUCCEEDED(scrolledView->GetClientData(clientData)))
{
+      nsISupports* data = (nsISupports*)clientData;
+
+      if (nsnull != data) {
+	 if (NS_FAILED(data->QueryInterface(NS_GET_IID(nsIFrame), (void
**)&fram
e))) {
+	   frame = nsnull;
+	 }
+      }
+    }



Address these 2 items, and jfrancis' concerns and you got an
sr=kin@netscape.com


-- I'm assuming the diffs for the following files are not part of the fix
   for bug 4302 so I didn't review them:

Index: directory/c-sdk/config/Makefile
Index: directory/c-sdk/ldap/clients/tools/Makefile
Index: directory/c-sdk/ldap/libraries/libiutil/Makefile
Index: directory/c-sdk/ldap/libraries/libldif/Makefile
Index: directory/c-sdk/ldap/libraries/libssldap/Makefile
Index: editor/libeditor/base/nsEditorCommands.cpp
Index: editor/libeditor/base/nsEditorCommands.h
Index: layout/base/public/nsIPresShell.h
Index: layout/html/base/src/nsFrame.cpp
Blocks: majorbugs
*** Bug 164199 has been marked as a duplicate of this bug. ***
isn't this fixed already?
marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Verified fixed in today's OS/2 trunk, which is very broken otherwise.
Oops, make that 99% fixed. The last PgUp should move cursor to 0,0, but only
goes to home row, not home column.
The patch for the Shift+PgUp/Dn keybindings wasn't checked in yet
(http://bugzilla.mozilla.org/attachment.cgi?id=82480&action=view)

Please check it in too.
Also I don't see the behavior which was mentioned in the comment #217 on Linux. 
Here the last PgUp moves the caret to (0,0) position.

I have checked in the patches which were not checked in but part of this fix. 
Shifted keybindings should work as should platform particular issues.
This fix looks to be responsible for a regression causing Page Up and Page Down
not to work in Browser (bug 165267).  That bug appeared sometime between
2002-08-26-21 (where it does not appear) and 2002-08-28-08 (where it does).
*** Bug 165320 has been marked as a duplicate of this bug. ***
I don't think this bug is the culprit. When only the first patch was checked in
the PgUp/Dn worked fine here in my browser from 20020827??. The second patch
checked in adds only new keybindings for Shift+PgUp/Dn and couldn't cause the
regression.

With the 2002082808 build the Shift-PgUp/Dn now correctly selects the text in
textareas and without shift it works like before the last checkin. So I say
this bug is verified fixed on Linux 2002082808.
> The second patch checked in adds only new keybindings for Shift+PgUp/Dn

Wrong.  Look at
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1030541100&maxdate=1030542500
please.  What happened was:

1)  bindings were added to the various platformHTMLBindings.xml 
2)  bindings were removed from htmlBindings.xml

reverting change #2 fixes the pgup/pgdown problem.  I'm not sure why (And that
discussion is for bug 165267).  But yes, this bug is 100% certainly responsible
for the regression.
Sorry for that 
I thought only this attachment was checked in.

http://bugzilla.mozilla.org/attachment.cgi?id=82480&action=view
verified in 9/5 trunk. Reopen if anyone disagrees.

I am opening a new bug 166938

because now the scrollbar does not go all the way to begin/end
when moving the caret to the begin/end.

interested parties can cc: themselves on that bug directly.
Status: RESOLVED → VERIFIED
*** Bug 167444 has been marked as a duplicate of this bug. ***
*** Bug 169629 has been marked as a duplicate of this bug. ***
*** Bug 167441 has been marked as a duplicate of this bug. ***
*** Bug 175623 has been marked as a duplicate of this bug. ***
*** Bug 178009 has been marked as a duplicate of this bug. ***
*** Bug 178660 has been marked as a duplicate of this bug. ***
*** Bug 179644 has been marked as a duplicate of this bug. ***
No longer blocks: majorbugs
Has this regressed in Firefox 2?  I can't select text by clicking inside a page then pressing Shift-Page Down.
This bug is about _editor_ as the summary says.  In Firefox, that basically means a textarea.  At no point could you select random text on a page with shift-pgdown.
You need to log in before you can comment on or make changes to this bug.