13927 - [dogfood]Repaint problems when deleting parts of the document

Status: VERIFIED FIXED

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

Description

[kinmoz]

To reproduce:

1. Load the default editor test page in the editor testbed, or load a document
that is larger than the visible area of the editor window.

2. Make a selection that spans from somewhere in the middle of the document to
the very end of the document. The selection must be larger than can fit in the
visible are of the screen. (You have to scroll to see the entire selection.)

3. Scroll the editor window down to the end of the document, so that the end of
the selection is visible.

4. Hit the backspace key.


You should still see the selected contents that should have been deleted because
the view did not repaint properly. If you bring a different window in front of
the editor window, to expose or force the editor view to redraw, you will
actually see things repaint properly.

This bug is related to the description in bug #13440.

To fix this bug, nsScrollingView::ComputeScrollOffsets() must be modified to
call nsSCrollingView::ScrollTo() to keep the screen and cliprects in sync with
the location of the view.

ScrollTo() also needs to be modified to support NS_VMREFRESH_NO_SYNC when
scrolling so that we avoid synchronous repainting while still in the reflow
code.

Comment 1

[Patrick C. Beard]

The editor has lots of other problems right now, that will make a fix hard to
verify. I just tried to do "select all" and the editor just hung.

Comment 2

[kinmoz]

The problem [e-mail] mentioned is bug #14473, assigned to
[e-mail].

Comment 3

[buster]

added the dependancy so patrick will know when he can test his fix

Comment 4

[kinmoz]

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

Comment 5

[troy]

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

Comment 6

[leger]

Putting on [PDT+] radar.

Comment 7

[rickg]

Patrick is in the midst of moving into his new home, but promises an estimate
tonight.

Comment 8

[rickg]

Patrick says, "This looks harder, so I'll estimate a couple of days. Let's say
by Thursday".

Comment 9

[Patrick C. Beard]

I have some feedback. We're now using nsScrollPortView, which does nothing
in ComputeScrollOffsets(), and yet strange behavior results. A simple way to
observe this is to simply scroll the document a small amount, and delete
any text. The entire document will be refreshed, but in the totally wrong
position, at essentially (0, 0) scroll position. It's as if the scroll offsets
have been totally zeroed out, but I don't see any calls to the view manager
that do that. I'm a little bit stumped right now.

Comment 10

[Patrick C. Beard]

After a little more research, the reason for this strange behavior is a call
to nsView:SetPosition(0, 0) that essentially undoes scrolling. Something needs
to be done to inhibit this positioning. This is not a view manager bug.

Here's the stack crawl:

  08242450    PPC  196D4EEC
nsHTMLEditorLog::DeleteSelection(nsIEditor::ESelectionCollapseDirection)+D4EEC
  08242400    PPC  196643C0
nsHTMLEditor::DeleteSelection(nsIEditor::ESelectionCollapseDirection)+643C0
  082422C0    PPC  1964C8B0
nsEditor::DeleteSelectionImpl(nsIEditor::ESelectionCollapseDirection)+4C8B0
  08242270    PPC  1963892C  nsEditor::Do(nsITransaction*)+3892C
  082421C0    PPC  1AF5CBB8  nsTransactionManager::Do(nsITransaction*)+5CBB8
  08242170    PPC  1AF5F17C
nsTransactionManager::BeginTransaction(nsITransaction*)+5F17C
  08242120    PPC  1AF5FB78  nsTransactionItem::Do()+5FB78
  082420E0    PPC  196336B4  EditAggregateTxn::Do()+336B4
  08242050    PPC  1962C4C4  DeleteTextTxn::Do()+2C4C4
  08241FE0    PPC  1977CE84  nsTextNode::DeleteData(unsigned int, unsigned int)+
7CE84
  08241FA0    PPC  197792C0  nsGenericDOMDataNode::DeleteData(unsigned int,
unsigned int)+792C0
  08241EC0    PPC  19779538  nsGenericDOMDataNode::ReplaceData(unsigned int,
unsigned int, const nsString&)+79538
  08241E30    PPC  1977DF40  nsTextNode::SetText(const unsigned short*, int,
int)+7DF40
  08241DF0    PPC  1977B28C  nsGenericDOMDataNode::SetText(const unsigned short*,
int, int)+7B28C
  08241DA0    PPC  19787540  nsDocument::ContentChanged(nsIContent*, nsISupports*
)+87540
  08241D50    PPC  197A6F8C  PresShell::ContentChanged(nsIDocument*, nsIContent*,
nsISupports*)+A6F8C
  08241D00    PPC  197A17D4  PresShell::ExitReflowLock(int, int)+A17D4
  08241CC0    PPC  197A52D4  PresShell::ProcessReflowCommands()+A52D4
  08241C20    PPC  198526A0  nsHTMLReflowCommand::Dispatch(nsIPresContext*,
nsHTMLReflowMetrics&, const nsSize&, nsIRenderingContext&)+526A0
  08241B00    PPC  1977F4F0  nsContainerFrame::PositionFrameView(nsIPresContext*,
nsIFrame*, nsIView*)+7F4F0
  08241AA0    PPC  1A730408  nsViewManager::MoveViewTo(nsIView*, int, int)+30408

Comment 11

[Patrick C. Beard]

Inserting text also exhibits the bug. Anything that causes reflow I'd wager.

Comment 12

[kinmoz]

Accepting bug.

Comment 13

[kinmoz]

The bug [e-mail] refers to above is bug #20107. I just checked in a
fix for this today.

I can't seem to reproduce this bug anymore with either Gfx or Native scrollbars.
I seem to remember a checkin a while ago that said something to the effect of we
now damage a larger area when the document is resized ... this may be what
actually fixed the problem.

I'm going to mark this bug fixed. QA can reopen it if they can reproduce it with
another test case.

By the way, the default editor test page referred to above is now accessed by
starting the editor and selecting Debug->Composer With Test Page from the menus.

Comment 14

[sujay]

will verify in 12/1 build.

Comment 15

[troy]

When you talk about the fix that damages a larger area, I think you're referring
to the view manager change referred to in bug #19256.

That may have fixed the problem you mention, but it was a regression that caused
us to do too much painting and it needs to get restored to the way it was.

So I wouldn't count on that change to fix this problem

Comment 16

[rubydoo123]

hate to do this after the fact but, linking to 12658, composer pdt+ bug tracking

Comment 17

[kinmoz]

The change troy refers to above is:

    mozilla/view/src/nsViewManager.cpp   revision 3.131

It turns out that the 3.131 change actually masked this bug as troy suggests. I
confirmed this by backing out the 3.131 changes in my copy of nsViewManager.cpp
(revision 1.135). Without the 3.131 changes, I had rendering problems with both
Native and Gfx scrollbars.

But, it turns out that beard checked in some changes to nsViewManager.cpp
(revision 3.136) lastnight that seems to fix the problem even when I back out
his 3.131 changes.

Comment 18

[sujay]

verified in 12/1 build.