291880 - the selected text does not paint selection, if the mouse goes over or goes out

Status: RESOLVED DUPLICATE of bug 371839

(Core :: DOM: Selection, defect)

Description

[Hideo Saito]

The selected text of the positioned frame does not paint selection, if the mouse
goes over or goes out.

for reproduce,
1. select several characters of "aaaaa" on testcase
2. if the mouse goes out, the selection disappears
3. if the window is redisplayed, the selection appears
4. if the mouse goes over "aaaaa",  the selection disappears again

This report came from http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=4405.

Comment 1

[Hideo Saito]

Attachment: testcase

Comment 2

[Hideo Saito]

Attachment: test patch

The positioned frame dose not have selected status, since the positioned frame
is newly created, when the mouse goes over and goes out.

Comment 3

[Hideo Saito]

Attachment: patch

Comment 4

[Hideo Saito]

Attachment: patch

roc, please give me your advice.

Every time the mouse goes over the positioned frame, the positioned inline
frame newly created using |NS_NewPositionedInlineFrame|. Every time the mouse
goes out of the positioned frame, the inline frame newly created using
|NS_NewInlineFrame|. In order to set |NS_FRAME_SELECTED_CONTENT| for child
frames of the newly created inline frame, I called |RepaintSelection|.

This new status prevents |RepaintSelection| from being called repeatedly.
Can I add new status |NS_FRAME_REPAINT_SELECTION| to |mState| of nsIFrame.h?

Comment 5

[Hideo Saito]

Attachment: patch

I try to search for a selected frame in the child frame and pass the status to the new child frame.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 200564 [details] [diff] [review]
patch

Quite apart from trying to dereference null, I think this is the wrong approach.  What about when new content is added?  Don't we need to repaint selections then?  And do we really want to repaint selection on every single mutation like this, even when batching (eg processing style reresolves)?

What problem are we really trying to solve?  The fact that selection state is stored in frames but selection is not watching for frame tree changes?

Comment 7

[Hideo Saito]

Attachment: patch

I think this approach is very simple, the changes look at the frame tree for the selection and call RepaintSelection();

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is potentially a significant slowdown.

Should we be setting NS_FRAME_SELECTED_CONTENT if necessary when constructing any frame? Perhaps in InitAndRestoreFrame?

Comment 9

[Hideo Saito]

Attachment: patch

> This is potentially a significant slowdown.

I think this change decreases the slowdown because the change to look at the frame tree can be included in DoDeletingFrameSubtree().

> Should we be setting NS_FRAME_SELECTED_CONTENT if necessary when constructing
> any frame? Perhaps in InitAndRestoreFrame?

Recreated frames only are looked at setting NS_FRAME_SELECTED_CONTENT.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What about plain old frame creation, like when content is inserted into an element that's selected?

Comment 11

[Hideo Saito]

Attachment: testcase for generated content

> What about plain old frame creation, like when content is inserted into an
> element that's selected?

If what is the meaning is to generate a content as the testcase, the selection is kept even if the mouse moves, if the patch is applied.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, I mean what if someone does .createElement() and then .appendChild() to a node which is selected, is the new content selected?

Comment 13

[Hideo Saito]

Attachment: testcase using .appendChild()

> No, I mean what if someone does .createElement() and then .appendChild() to a
> node which is selected, is the new content selected?

In the case of this testcase, if select '[]' and click 'ok', the appended content is not painted.

Comment 14

[Hideo Saito]

Attachment: patch

I added a change to nsCSSFrameConstructor::ContentAppended() in order to look at the selection of the parent frame which is appended the new content, although I was not able to merge HasSelectedFrame(). The change to nsTextFrame::SetSelected() is necessary to prevent to set NS_FRAME_SELECTED_CONTENT without actual selection.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not really sure what to do here. We need to set NS_FRAME_SELECTED_CONTENT if necessary on every new frame and I don't think this patch is the right way to do it:

+      if (IsInlineFrame(parentFrame) && HasSelectedFrame(parentFrame)) {
+        nsFrameSelection* frameSelection = mPresShell->FrameSelection();
+        frameSelection->RepaintSelection(nsISelectionController::SELECTION_NORMAL);

There's still a problem here when parentFrame is not inline. And what about the other selection types? And what about selection in, say, a textarea whose frames go away and then are recreated?

It won't even work in general; I think we have a problem if someone selects some content, then makes the entire container display:none, and then later restores its CSS display property. We need to set NS_FRAME_SELECTED_CONTENT as necessary on every new frame, no matter why it's created.

I'm not really sure what the best fix for this is. Maybe after every construction of a frame (sub)tree we should call a method on nsFrameSelection (either the document's one or the text frame's one) that calls Repaint for every selection type. That might not be too bad.

Comment 16

[Hideo Saito]

Attachment: patch

Comment 17

[Hideo Saito]

Attachment: patch

When the window is focused, the selections on the text appear as to each testcase, so that I searched some appropriate places to call |RepaintSelection|. The change of |ContentInserted| is for first testcase, and the change of |ContentAppended| is for other testcase.

Comment 18

[Hideo Saito]

Comment on attachment 332080 [details] [diff] [review]
patch

roc, could you review this patch?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we can't check the frame type here. We might have created something that's, say, a block containing some content that's selected.

And we can't just handle the NORMAL selection. We should repaint all selection types where a selection exists. I guess we need a RepaintAllSelections method.

Comment 20

[Hideo Saito]

Attachment: patch

How about appending a new function of LoadingDocument() like as this patch in order to decrease the cost? I think that any selections as to this changes are ignored while loading the document.

Comment 21

[Boris Zbarsky [:bzbarsky]]

So why is incorrect behavior OK while loading the document?

Comment 22

[Hideo Saito]

Attachment: patch

How about this patch? I removed the condition as to while loading the document because the selections are available meanwhile. I think that this approach should be denied if this patch causes slow down for some documents.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The code added to ContentAppended and ContentInserted could be shared in a helper function.

But I'm worried about performance here. Load a long text document. When a lot of it has loaded, do "select all". It seems to me that this patch will slow down the loading of the rest quite a bit.

What we need to do is, for each range in each selection, test whether the appended/inserted content is in the range. If it is, do a "Repaint" for that range, otherwise do nothing.

Comment 24

[Boris Zbarsky [:bzbarsky]]

If we want to do that, we might need to also switch to storing the selection ranges in some sort of sorted order....

Comment 25

[Hideo Saito]

Attachment: patch

I appended a new interface isSelectedFrame and using callback in order not to call RepaintAllSelections many times. nsTypedSelection::isSelectedFrame is based on nsTypedSelection::selectFrames. However, it seems that the isSelectedFrame has many costs yet and it is called many times. Is this approach quite bad?

Comment 26

[Hideo Saito]

Comment on attachment 339609 [details] [diff] [review]
patch

Sorry, it's my mistake.

Comment 27

[Hideo Saito]

Attachment: patch

Comment 28

[Hideo Saito]

The following code was needed in attachment 339611 [details] [diff] [review].

--- ./layout/base/nsCSSFrameConstructor.h.org	Sun Mar 30 11:08:07 2008
+++ ./layout/base/nsCSSFrameConstructor.h	Sat Sep  6 22:37:02 2008
@@ -52,8 +52,9 @@
 #include "nsHashKeys.h"
 #include "nsThreadUtils.h"
 #include "nsPageContentFrame.h"
 #include "nsIViewManager.h"
+#include "nsIReflowCallback.h"
 
 class nsIDocument;
 struct nsFrameItems;
 struct nsAbsoluteItems;
@@ -79,9 +80,9 @@ typedef void (PR_CALLBACK nsLazyFrameCon
 
 class nsFrameConstructorState;
 class nsFrameConstructorSaveState;
   
-class nsCSSFrameConstructor
+class nsCSSFrameConstructor : public nsIReflowCallback
 {
 public:
   nsCSSFrameConstructor(nsIDocument *aDocument, nsIPresShell* aPresShell);
   ~nsCSSFrameConstructor(void) { }
@@ -97,8 +98,13 @@ public:
 private: 
   // These are not supported and are not implemented! 
   nsCSSFrameConstructor(const nsCSSFrameConstructor& aCopy); 
   nsCSSFrameConstructor& operator=(const nsCSSFrameConstructor& aCopy); 
+
+  PRPackedBool mPostedReflowCallback:1;
+  void RepaintAllSelections(nsIFrame* aFrame);
+  virtual PRBool ReflowFinished();
+  virtual void ReflowCallbackCanceled();
 
 public:
   // XXXbz this method needs to actually return errors!
   nsresult ConstructRootFrame(nsIContent*     aDocElement,

Comment 29

[Hideo Saito]

It seems that this bug already fixed on the trunk for seamonkey 2.1a1pre, but not yet fixed on latest branch for seamonky-2.0.5pre.

Comment 30

[Hideo Saito]

As a result of my checking on nightly build,

NG: seamonkey-2.1a1pre.en-US.linux-i686 in 2009-07-26-06-common-central-trunk
OK: seamonkey-2.1a1pre.en-US.linux-i686 in 2009-07-28-10-common-central-trunk

But I was not able to find out the reason why this bug was fixed, although I saw http://hg.mozilla.org/comm-central/log/.

Comment 31

[WADA:World Anti-bad-Duping Agency]

Pushlog.
> http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-07-26&enddate=2009-07-28