Closed Bug 7354 Opened 25 years ago Closed 25 years ago

Arrow Keys don't work well.

Categories

(Core Graveyard :: GFX, defect, P3)

Product:
Core Graveyard
Component:
GFX
Platform:
x86
Other
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: niles, Assigned: ramiro)

References

(
URL
)

Details

(Whiteboard: [Perf]) 

There are two problems with using arrow keys in the M6 release of Mozilla under Linux (Unix):

1) The page will not render correctly when scrolling with the arrow keys.  Text lines appear
     twice or more and the page becomes unreadable.  This does not happen if you exclusively
     use the mouse for scroll control.  In fact the unreadable page can be re-rendered by using
     the mouse.

2) Too many down key events get registered for "up" & "down" keys.  After hold down the key for
     even just one second the browser with continue to scroll for almost 10 seconds.  This could be
     attributed to performance. I.e. if the browser scrolled faster it could keep up with the incomming
     events.
Status: NEW → ASSIGNED
This is really a problem with the priority of key events vs. update events. This
also happens on the Mac OS if you hold down the down/up arrow key while
scrolling. Gaps are left that aren't updated correctly. I think the right fix
for this is to force the repaint to occur before processing another key event.
This is how the scrollbar code works. So, it's technically a layout bug, since
the event handling code is over there.

Here's a patch to nsEventStateManager.cpp that forces the repaint to happen
immediately after the scroll, and this fixes the problem on Mac OS. I think this
is an XP fix, but I don't like having to walk up the view hierarchy to do it. As
the comment indicates, a simple call to nsIViewManager::Composite() is
insufficient, since it only works if invalidation is done through the view
manager, and scrolling isn't. If you can, give this patch a try.

Index: nsEventStateManager.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/events/src/nsEventStateManager.cpp,v
retrieving revision 1.36
diff -c -2 -r1.36 nsEventStateManager.cpp
*** nsEventStateManager.cpp	1999/05/19 23:28:18	1.36
--- nsEventStateManager.cpp	1999/05/30 19:29:23
***************
*** 209,212 ****
--- 209,228 ----
                nsKeyEvent * keyEvent = (nsKeyEvent *)aEvent;
                sv->ScrollByLines((keyEvent->keyCode == NS_VK_DOWN) ? 1 : -1);
+
+               // force the update to happen now?
+               nsIViewManager* vm;
+              	if (NS_OK == aView->GetViewManager(vm)) {
+              		// I'd use Composite here, but it doesn't always work.
+                 // vm->Composite();
+                 nsIView* rootView = nsnull;
+                 if (NS_OK == vm->GetRootView(rootView) && nsnull != rootView)
{
+               	  nsIWidget* rootWidget = nsnull;
+               		if (NS_OK == rootView->GetWidget(rootWidget) && nsnull
!= rootWidget) {
+                     rootWidget->Update();
+                     NS_RELEASE(rootWidget);
+                   }
+               	}
+                 NS_RELEASE(vm);
+               }
              }
            }

I can check this in if it's deemed a general enough fix.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Target Milestone: M7
Checked into mozilla/layout/events/src/nsEventStateManager.cpp v1.37. Please
verify on linux.
Status: RESOLVED → REOPENED
still broken under linux
Blocks: 4299
this seems to have fixed the pageup/page down problems though.
Blocks: 5669
Assignee: beard → ramiro
Status: REOPENED → NEW
Handing bug to ramiro for linux-specific problems.
Putting on [Perf] radar
Resolution: FIXED → ---
Whiteboard: [Perf]
*** Bug 7351 has been marked as a duplicate of this bug. ***
*** Bug 7355 has been marked as a duplicate of this bug. ***
*** Bug 7651 has been marked as a duplicate of this bug. ***
This bug now has a whole article about it!
http://www.osopinion.com/Opinions/GlennMullikin/GlennMullikin10.html
*** Bug 7360 has been marked as a duplicate of this bug. ***
*** Bug 6087 has been marked as a duplicate of this bug. ***
*** Bug 7013 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
Target Milestone: M7 → M8
marking assigned m8
*** Bug 4299 has been marked as a duplicate of this bug. ***
Take a look at bug #6014
This issue is occuring in the June 30th (1999063009)
This is what I could find.. the problem is that Scroll() is called twice:

When you use keys to scroll, ScrollByLine() and ScrollByPage() are called, these
in turn call ScrollTo().

ScrollTo() instructs the scrollbar to move to a specific position, and then
calls Scroll(). In Linux, the calls to move the scrollbar seems to also trigger
a scrollbar move event. The scrollbar move event causes HandleScrollEvent() to
also call Scroll() resulting in a double-repaint.

This can be fixed by commenting out the call to Scroll() in the ScrollTo()
event. I have no idea what the result will be on non-Linux platforms.

The previous patch to force the repaint after a key is pressed seems to break
this fix as well, but reverting his patch will break the delayed-stop problem
when holding down a button.

What really needs to happen is to figure out how to stop it from spitting out
the scrollbar move event.

Index: view/src/nsScrollingView.cpp
===================================================================
RCS file: /cvsroot/mozilla/view/src/nsScrollingView.cpp,v
retrieving revision 3.104
diff -c -r3.104 nsScrollingView.cpp
*** nsScrollingView.cpp 1999/05/19 22:14:10     3.104
--- nsScrollingView.cpp 1999/07/01 05:49:24
***************
*** 1283,1289 ****
--- 1283,1292 ----
      mOffsetY = aY;
    }

+ // This fixes bug #7354
+ #if 0
    Scroll(scrolledView, dx, dy, t2p, 0);
+ #endif

  #if 0
    if (dx || dy)
Here is an even better fix which shouldn't break other platforms. What we do is
remove the saving of mOffsetX and mOffsetY so that if scrollv->SetPosition() or
scrollh->SetPosition() end up changing those values, the deltas will be 0 and
the duplicate Scroll() call won't do anything. This seems to work perfectly on
my tree:

Index: view/src/nsScrollingView.cpp
===================================================================
RCS file: /cvsroot/mozilla/view/src/nsScrollingView.cpp,v
retrieving revision 3.104
diff -c -r3.104 nsScrollingView.cpp
*** nsScrollingView.cpp 1999/05/19 22:14:10     3.104
--- nsScrollingView.cpp 1999/07/01 07:00:35
***************
*** 1229,1241 ****
          aY = 0;

        // Move the scrollbar's thumb
-
-       PRUint32  oldpos = mOffsetY;
        PRUint32  newpos = NSIntPixelsToTwips(NSTwipsToIntPixels(aY, t2p), p2t);

        scrollv->SetPosition(newpos);

!       dy = NSTwipsToIntPixels((oldpos - newpos), t2p);

        NS_RELEASE(scrollv);
      }
--- 1229,1239 ----
          aY = 0;

        // Move the scrollbar's thumb
        PRUint32  newpos = NSIntPixelsToTwips(NSTwipsToIntPixels(aY, t2p), p2t);

        scrollv->SetPosition(newpos);

!       dy = NSTwipsToIntPixels((mOffsetY - newpos), t2p);

        NS_RELEASE(scrollv);
      }
***************
*** 1257,1269 ****
          aX = 0;

        // Move the scrollbar's thumb
-
-       PRUint32  oldpos = mOffsetX;
        PRUint32  newpos = NSIntPixelsToTwips(NSTwipsToIntPixels(aX, t2p), p2t);

        scrollh->SetPosition(newpos);

!       dx = NSTwipsToIntPixels((oldpos - newpos), t2p);

        NS_RELEASE(scrollh);
      }
--- 1255,1265 ----
          aX = 0;

        // Move the scrollbar's thumb
        PRUint32  newpos = NSIntPixelsToTwips(NSTwipsToIntPixels(aX, t2p), p2t);

        scrollh->SetPosition(newpos);

!       dx = NSTwipsToIntPixels((mOffsetX - newpos), t2p);

        NS_RELEASE(scrollh);
      }
Blocks: 9164
jordy:

Thanks very much for tracking this down.  I tested your fix, and yes, it fixes
the problem on unix - which is great.

Unfortunately, when I tried it on windows, it causes bad things - like crashes.

Im working with Patrick Beard to munge the fix so that it works on XP code.

We will probably checkin something soon.

Thanks again.

Adding beard to cc list.  Taking rickg out of cc list.
beard:

When I try this patch on linux, it works as advertised.  On windows, I get the
following crash:

nsRangeList::HandleKeyEvent(nsRangeList * const 0x016d55d0, nsGUIEvent *
0x0012fccc) line 1062 + 66 bytes
PresShell::HandleEvent(PresShell * const 0x016d56a4, nsIView * 0x016ff8b0,
nsGUIEvent * 0x0012fccc, nsEventStatus & nsEventStatus_eIgnore) line 2058
nsView::HandleEvent(nsView * const 0x016ff8b0, nsGUIEvent * 0x0012fccc, unsigned
int 8, nsEventStatus & nsEventStatus_eIgnore, int & 0) line 833
nsView::HandleEvent(nsView * const 0x016fe4a0, nsGUIEvent * 0x0012fccc, unsigned
int 8, nsEventStatus & nsEventStatus_eIgnore, int & 0) line 818
nsView::HandleEvent(nsView * const 0x016fe530, nsGUIEvent * 0x0012fccc, unsigned
int 8, nsEventStatus & nsEventStatus_eIgnore, int & 0) line 818
nsView::HandleEvent(nsView * const 0x016d3780, nsGUIEvent * 0x0012fccc, unsigned
int 28, nsEventStatus & nsEventStatus_eIgnore, int & 0) line 818
nsViewManager::DispatchEvent(nsViewManager * const 0x016d3a40, nsGUIEvent *
0x0012fccc, nsEventStatus & nsEventStatus_eIgnore) line 1736
HandleEvent(nsGUIEvent * 0x0012fccc) line 67
nsWindow::DispatchEvent(nsWindow * const 0x016fe374, nsGUIEvent * 0x0012fccc,
nsEventStatus & nsEventStatus_eIgnore) line 493 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012fccc) line 518
nsWindow::DispatchKeyEvent(unsigned int 133, unsigned short 0, unsigned int 40)
line 1924 + 15 bytes
nsWindow::OnKeyDown(unsigned int 40, unsigned int 80) line 2117
nsWindow::ProcessMessage(unsigned int 256, unsigned int 40, long 1095761921,
long * 0x0012fef0) line 2442 + 40 bytes
nsWindow::WindowProc(HWND__ * 0x154d01d2, unsigned int 256, unsigned int 40,
long 1095761921) line 566 + 27 bytes
USER32! 77e71268()

Im hesitant to checkin #ifdef XP_UNIX code to te nsScrollingView.  Do you have
any ideas ?

Kevin ?
I'd almost recommend reorganizing the scrolling code completely to clean out the
duplicate code...

Remove the keyboard-only lineup, linedown, page up, page down code and have the
keyboard move the scrollbar to trigger an event as if the scrollbar. Then put
the rest of the code in the event handler.

This would add a little extra latency and a bit of CPU, but it would remove
the duplicate code and simplify the scrolling code itself significantly.
This patch seems to work fine with both my Windows NT and Mac builds (as of
today).
Target Milestone: M8 → M9
m9
*** Bug 10476 has been marked as a duplicate of this bug. ***
-> m10
Status: ASSIGNED → RESOLVED
Closed: 25 years ago → 25 years ago
Resolution: --- → FIXED
Jordy,

Once again, thanks for your fix.  Unfortunately, it was breaking on win32.

In any case, your fix gave some clues as to what was wrong.

Ive checked in a proper fix to the gtk scrollbar code that should fix all the
gtk specific keyboard scrolling bugs.

Marking fixed.
Status: RESOLVED → VERIFIED
This is fixed in the Aug 24 Linux build.
*** Bug 6014 has been marked as a duplicate of this bug. ***
Using the 07-Sep-99 build on Linux, I see a problem in which pressing the down
arrow key to scroll just makes the page jump around a lot - the page is never
actually scrolled down very far at all, and keeps returning to the top. If
anyone assigned to this bug can reproduce that, should this bug be reopened or a
new one filed? Thanks!
I say file a new bug.  And file it against beard@netscape.com.

Its an XP bug, ive seen it happen in windows too.

Try this:

www.theonion.com on a windows build and then try the same up/down key
scrolling.  Try it a few times, sometimes it takes more than one try.
New bug is bug 13369, filed against beard@netscape.com. Thanks, and apologies
for the bug mail.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.