10921 - Move one line down results in reformting page and scroll bar moving

Status: VERIFIED FIXED

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

Description

[bijals (gone)]

Steps to Reproduce:
1) Launch the Editor application from the Task menu in Navigator
2) In the test document inside the Editor application, put cursor on top line
3) Use arrow keys to move cursor down
4) Notice that document text moved up, scroll bar moved, and the text in
document is temporarily miss aligned

Actual Results: Document scrolls

Expected Results: No scrolling until cursor gets to the last visible line in the
document.

Build Date & Platform Bug Found: 1999-07-30-11-M9 on Win95

Comment 1

[buster]

This bug has 2 causes.

1. Mike's event handler for VK_UP and VK_DOWN don't cancel the keystroke if they
handle it.  I have fixed that and I'll check it in.  As a secondary fix, I also
make these handlers have effect only if the caret is enabled. Otherwise, default
processing (scrolling) should occur, regardless of the selection state.

2. Even with the rangelist consuming the event
(by setting aEvent->flags |= nsEventStatus_eConsumeNoDefault)
the event handling code in nsPresShell completely ignores the event result.
The code that needs to be fixed is annotated below:

    if (mSelection && aEvent->eventStructType == NS_KEY_EVENT)
    {
      mSelection->EnableFrameNotification(PR_FALSE);
      mSelection->HandleKeyEvent(aEvent);                <--- event consumed
      mSelection->EnableFrameNotification(PR_TRUE);
    }
    nsIEventStateManager *manager;
    nsIContent* focusContent = nsnull;
    if (NS_OK == mPresContext->GetEventStateManager(&manager)) {
      if (NS_IS_KEY_EVENT(aEvent)) {
        manager->GetFocusedContent(&focusContent);
      }
      frame->GetFrameForPoint(aEvent->point, &mCurrentEventFrame);
      NS_IF_RELEASE(mCurrentEventContent);
      if (GetCurrentEventFrame() || focusContent) {
        rv = manager->PreHandleEvent(*mPresContext,
               aEvent, mCurrentEventFrame, aEventStatus, aView);

        if ((GetCurrentEventFrame() || focusContent) && NS_OK == rv) {
          if (focusContent) {
            rv = focusContent->HandleDOMEvent(*mPresContext,
                nsEvent*)aEvent, nsnull,
                NS_EVENT_FLAG_INIT,        <-- INIT flag erases previous result!
                aEventStatus);
          }

I will check in the fixes to PresShell and RangeList, but making this code
properly respect the event result is beyond me.  I don't know all the
ramifications of putting checks in place here.

Assigned to joki, cc mjudge.  Set to M9 because this is a real usability
problem.

Comment 2

[buster]

after talking to Mike, I decided not to check in this code after all.  I'll let
him do the right thing. Here's the code to check if the caret is visible:
NS_IMETHODIMP
nsRangeList::HandleKeyEvent(nsGUIEvent *aGuiEvent)
{
  if (!aGuiEvent)
    return NS_ERROR_NULL_POINTER;
  STATUS_CHECK_RETURN_MACRO();

  // only do navigation if the caret is visible       <-- Mike, new code here
  nsCOMPtr<nsIPresContext> context;
  nsCOMPtr<nsIPresShell> shell;
  nsresult result = mTracker->GetPresContext(getter_AddRefs(context));
  if (NS_FAILED(result) || !context)
    return result;
  result = context->GetShell(getter_AddRefs(shell));
  if (NS_FAILED(result) || !shell)
    return result;
  PRBool caretEnabled=PR_FALSE;
  result = shell->GetCaretEnabled(&caretEnabled);
  if (NS_FAILED(result)) { return result; }
  if (PR_FALSE==caretEnabled)
  {
    return NS_OK;  // if there is no caret visible, don't move the selection,
and allow default processing
  }

Comment 3

[chris hofmann]

joki at w3c

Comment 4

[lchiang]

adding myself to cc: list.

Comment 5

[joki (gone)]

Mike, I think that, if this still exists, its probably yours.  In bringing up
the Editor with its built in test case, the main problems I see are that we
correctly scroll until we hit a certain point in the document at which point
the cursor will no longer move through the doc.  But since it works until that
point I don't think this is event related.

Comment 6

[mjudge]

works great now. no scrolling in editor... sometimes get stuck but it will be
fixed THIS problem is done though.

Comment 7

[sujay]

verified in 9/21 build...

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5582aa55b910
[fenix] For https://github.com/mozilla-mobile/fenix/issues/10921 - Integrate BrowserThumbnails feature into the tabs tray (https://github.com/mozilla-mobile/fenix/pull/11093)