10673 - [DOGFOOD] Selection refresh problem after apply style.

Status: VERIFIED FIXED

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

Description

[buster]

there are editor operations that do not set the selection properly, (or
seem to not set the selection properly, when in fact the selection is
fine) making the editor hard to use.  Those the the cc list, please add specific
examples to this bug.

1) setting/removing text properties with the toolbar buttons.  selection is
always set correctly, but sometimes it disappears.  If you use the key bindings,
you see that selection is in the correct place.

The problem here is nsDocument::SetDisplaySelection should force a repaint if
the selection display boolean is changed.

2) Set text property across a block boundary, an extra character or two gets
selected.  This is bug 10654.

Comment 1

[Simon Fraser [no longer active]]

Adjust summary to be more accurate.

Comment 2

[Simon Fraser [no longer active]]

kin wants this.

Comment 3

[buster]

To verify that this was just a paint problem, I added the following code
reminicent of the HackForceRedraw stuff.  It works, but it isn't right. If the
root cause of the problem lies elsewhere, this code could be put in place as
a temporary measure.  But that defeats the purpose of incremental painting.
Maybe what should happen is one or more of these:
1) toolbar buttons shouldn't take focus away, or
1a) we should be able to detect when we're losing focus to chrome, and not
disable/enable caret and selection.  See bug 10678.
2) when selection is enabled/disabled, the document should notify someone, and a
repaint of the selected region only should be triggered.
----------------------------------------------------------------------------
nsresult
nsTextEditorFocusListener::Focus(nsIDOMEvent* aEvent)
{
  // turn on selection and caret
  if (mEditor)
  {
    nsCOMPtr<nsIEditor>editor = do_QueryInterface(mEditor);
    if (editor)
    {
      nsCOMPtr<nsIPresShell>ps;
      editor->GetPresShell(getter_AddRefs(ps));
      if (ps)
      {
        ps->SetCaretEnabled(PR_TRUE);

        nsCOMPtr<nsIDOMDocument>domDoc;
        editor->GetDocument(getter_AddRefs(domDoc));
        if (domDoc)
        {
          nsCOMPtr<nsIDocument>doc = do_QueryInterface(domDoc);
          if (doc)
          {
            doc->SetDisplaySelection(PR_TRUE);
          }
        }
// begin hack repaint                <-- begin added code
        nsCOMPtr<nsIViewManager> viewmgr;
        ps->GetViewManager(getter_AddRefs(viewmgr));
        if (viewmgr) {
          nsIView* view;
          viewmgr->GetRootView(view);			// views are not
refCounted
          if (view) {
            viewmgr->UpdateView(view,nsnull,NS_VMREFRESH_IMMEDIATE);
          }
// end hack repaint               <-- end added code
        }
      }
    }
  }
  return NS_OK;
}

Comment 4

[kinmoz]

As an experiment, I wrote
a method called nsTextEditorFocusListener::HackForceSelectionRepaint() that
clears the selection and resets it back to what it was before it was cleared,
and added a call to it in nsTextEditorFocusListener::Focus(). This fools the
selection code into repainting itself. Causing the block that contains the
selection to repaint, but not the entire document, so it's a bit better than the
old HackForceRedraw. I've only tested this on windows.


nsresult
nsTextEditorFocusListener::HackForceSelectionRepaint()
{
  nsresult result;
  nsCOMPtr<nsIDOMSelection> selection;
  nsCOMPtr<nsIEditor> editor = do_QueryInterface(mEditor);

  if (!editor)
    return NS_ERROR_NULL_POINTER;

  result = editor->GetSelection(getter_AddRefs(selection));

  if (NS_FAILED(result))
    return result;

  if (!selection)
    return NS_ERROR_NULL_POINTER;

  PRInt32 rcount = 0;

  result = selection->GetRangeCount(&rcount);

  if (NS_FAILED(result))
    return result;

  if (rcount < 1)
    return NS_OK;

  // Clone all the selection ranges!

  nsCOMPtr<nsISupportsArray> arr;

  result = NS_NewISupportsArray(getter_AddRefs(arr));

  if (NS_FAILED(result))
    return result;

  if (!arr)
    return NS_ERROR_NULL_POINTER;

  nsCOMPtr<nsIDOMRange> range;
  nsCOMPtr<nsIDOMRange> newRange;
  nsCOMPtr<nsISupports> isupp;
  PRInt32 i;

  for (i = 0; i < rcount; i++)
  {
    result = selection->GetRangeAt(i, getter_AddRefs(range));

    if (NS_FAILED(result))
      return result;

    if (!range)
      return NS_ERROR_NULL_POINTER;

    result = range->Clone(getter_AddRefs(newRange));

    if (NS_FAILED(result))
      return result;

    if (!newRange)
      return NS_ERROR_NULL_POINTER;

    isupp = do_QueryInterface(newRange);

    if (!isupp)
      return NS_ERROR_NULL_POINTER;

    result = arr->AppendElement(isupp);

    if (NS_FAILED(result))
      return result;
  }

  // Now clear the selection and add the cloned
  // ranges to the selection.

  result = selection->StartBatchChanges();

  if (NS_FAILED(result))
    return result;

  result = selection->ClearSelection();

  if (NS_FAILED(result))
  {
    selection->EndBatchChanges();
    return result;
  }

  for (i = 0; i < rcount; i++)
  {
    isupp = dont_AddRef(arr->ElementAt(i));

    if (!isupp)
    {
      selection->EndBatchChanges();
      return NS_ERROR_NULL_POINTER;
    }

    newRange = do_QueryInterface(isupp);

    if (!newRange)
    {
      selection->EndBatchChanges();
      return NS_ERROR_NULL_POINTER;
    }

    result = selection->AddRange(newRange);

    if (NS_FAILED(result))
      return result;
  }

  result = selection->EndBatchChanges();

  if (NS_FAILED(result))
    return result;

  return NS_OK;
}

Comment 5

[kinmoz]

We can forget about using the HackForceSelectionRepaint() method above.
Everytime it is called, it has the side effect of clearing the nsTextEditor's
TypeInState.

Comment 6

[kinmoz]

Looks like buster checked in the view hack repaint code he mentioned. Should I
mark this fixed? Or leave it open till we really fix the problem?

Comment 7

[buster]

leave it open until a real solution is found.  I still think that when selection
is enabled/disabled, the document should notify someone, and a repaint of the
selected region only should be triggered.

Comment 8

[buster]

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

Comment 9

[kinmoz]

Added a RepaintSelection() method to the PresShell. The old code, which used to
redraw the entire view in nsTextEditorFocusListener::Focus() and
nsTextEditorFocusListener::Blur(), has been commented out with USE_HACK_REPAINT
and replaced with calls to RepaintSelection().

Fix for bugs #7153, #10673, #12066, #12067, and #12793.

    editor/base/nsEditorEventListeners.cpp    revision 1.89
    editor/base/nsEditorEventListeners.h      revision 1.115
        - Added code to scroll the selection into view
          after processing key events.
        - Commented out the hack that redraws the entire
          view when the focus is gained and lost. Replaced
          the hack code with calls to RepaintSelection().

    layout/base/public/nsIFrameSelection.h    revision 1.115
    layout/base/public/nsIPresShell.h         revision 3.46
    layout/html/base/src/nsPresShell.cpp      revision 3.167
        - Added ScrollSelectionIntoView() and RepaintSelection() methods.

    layout/base/src/nsRangeList.cpp    revision 1.123
        - Added implementation for ScrollSelectionIntoView() and
          RepaintSelection().
        - Check for NULL primary frame in GetFocusNodeRect().

Comment 10

[sujay]

verified in 9/2 build.

Comment 11

[leger]

Putting on [PDT]+ radar.