(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.

ok ok

set milestone M10

fix summary

moving to m12

changing milestone to after dogfood, setting to M15

feature clean-up work for beta

moving this up to m14

setting keyword

Putting on PDT- radar for beta1. Would not hold beta for this.

move to M15; remove beta1 keyword since this has PDT- designation

M16 for now

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.

[nsbeta2+ until 5/16]

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

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

adding help wanted to the keywords

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

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

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

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]

*** 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.

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).

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

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

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)

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

*** 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.

*** 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.

*** 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

-->anthonyd

*** 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

--> mjudge owns selection and selection/caret navigation

*** 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.

removing nsbranch stuff...

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

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. ***

Marking nsbeta1+ per syd. Need to fix for composer usability.

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.

0.9.6 is gone -> 0.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. ***

*** 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

moving to 1.0

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

*** 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

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.

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

*** 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. ***

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.

See comment 123

> 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

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?

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.

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

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.

OK. I removed #ifdef.

For the patch for nsPresShell, I removed #ifdef and debug codes. This patch sometimes behave wrong as said above. Is it ok?

*** 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.

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

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 ;)

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 :-)

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.

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?

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. ***

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.

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 ;)

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

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

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

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

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

Mike, where does this bug stand?

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>

> 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. ***

*** 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.

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.

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.

ignore nsHRFrame and nsFrame.cpp in diff. those are for bug 159207.

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.

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.

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.

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.

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?

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.

Comment on attachment 95309 [details] [diff] [review] patch from /mozilla carrying over the r=

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

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

isn't this fixed already?

marking 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.