92217 - [reflow] text doesn't rewrap after becoming small enough to wrap

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P3)

Description

[Jesse Ruderman]

Mouse over the text in the testcase.  It becomes big when you mouse over it,
causing it to move to the next line.  But when you move the mouse again and it
reverts to its normal size, it doesn't jump back up to the line it was on before.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

May be related to bug 76117, checkbox label doesn't unwrap when resizing browser
window.  76117 is not consistently reproducable.

Comment 3

[anthonyd]

the extra line problem seems to only happen with tables

Comment 4

[karnaze (gone)]

Temporarily moving to future until a milestone can be assigned. 

Comment 5

[karnaze (gone)]

mass reassign to default owner

Comment 6

[Hideo Saito]

All/Alll

Comment 7

[Hideo Saito]

Attachment: patch

There are two problems as follows.

1. for the testcase, the mouseout does not cause line layout.
2. the mouse moves cause multipule mouseover and mouseout as follows.

   M	 
  +--+	  +M-+	     M	     +M-+     M is mouse, a box is a inline frame.
  |  |	  |  |	    +--+     |	| 
  +--+	  +--+	    +--+     +--+
	mouseover  mouseout  mouseover

For the multipule mouseover and mouseout, two variables are added to
nsInlineFrame class.

1. PRUint32 mEventTime;
2. enum mMouseOverState;

When a time lag(mEventTime) is less than 50 msec, since the multipule mouseover
and mouseout are accuring, a stylechange reflow is stopped.

Comment 8

[Hideo Saito]

Attachment: testcase for patch

Comment 9

[Hideo Saito]

> 1. PRUint32 mEventTime;
1. PRTime mEventTime;

Comment 10

[Bernd]

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

This patch is wrong. Its obvious that the text needs to wrap when then font
size increases, but it does not remove the line break, when the font size goes
back.

Comment 11

[Bernd]

Attachment: testcase to show that the patch is wrong

Comment 12

[Bernd]

this is a line issue

Comment 13

[rbs]

Re: Comment #7
> a stylechange reflow is stopped.

(no need for a timer, etc..., since reflows are coalesced.)

The stylechange reflow should in fact do the right thing here. If it is not,
then that's were the bug is (in the block code). The underlying problem seems
that the reflow is not marking the previous line dirty.

Comment 14

[Hideo Saito]

(In reply to comment #10)
> (From update of attachment 163710 [details] [diff] [review])
> This patch is wrong. Its obvious that the text needs to wrap when then font
> size increases, but it does not remove the line break, when the font size goes
> back.

The following code of the patch will reflow again, if oldFontSize and
newFontSize are not equal. I think the line break is removed.

+          if (needsDirty) {
+            aFrame->ReflowDirtyChild(aPresContext->GetPresShell(), nsnull);
+          }

Comment 15

[Hideo Saito]

(In reply to comment #13)
> Re: Comment #7
> > a stylechange reflow is stopped.
> 
> (no need for a timer, etc..., since reflows are coalesced.)

A timer and a status of mouseover are needed, because mouseover and mouseout are
repeated, it is necessary to stop it.

Comment 16

[Hideo Saito]

Mouseover and mouseout are repeated as follows. M is mouse, a box is a inline
frame. 

   M	 
  +--+	  +M-+	     M	     +M-+        M        +M-+    
  |  |	  |  |	    +--+     |  |       +--+      |  |    ...
  +--+	  +--+	    +--+     +--+       +--+      +--+
	mouseover  mouseout  mouseover mouseout  mouseover ...

Comment 17

[Hideo Saito]

For the testcase, the patch of comment #14 put back the text immediately when
the font size decreases, in result the jumping is repeated, since mouseover
causes the font size to increase and mouseout causes the font size to decrease.

Though this jumping is stopped by using a timer, is this behavior wrong?

Comment 18

[rbs]

> A timer and a status of mouseover are needed

Nope. Nor do you need to special-case the font-size there. The Style System is
already catering for that, by calculating the style difference and returning
'nsChangeHint_ReflowFrame', which means reflow the whole thing.

Here is what should happen. That frame that you are trying to reflow is queued
for reflow as follows. The mouseover/mouseout causes the font size to change,
i.e., it triggers a style re-resolution:

nsFrameManager::ReResolveStyleContext(
  ->call: aMinChange = CaptureChange()
    ->call: aChangeList->AppendChange(), the frame is enlisted in |aChangeList|.

After some further vodoo, Gecko will ultimately do:

nsCSSFrameConstructor::ProcessRestyledFrames(aChangeList)
  -> nsCSSFrameConstructor::StyleChangeReflow()
 
And this is where the frame is queued in a |reflow command|, i.e., you don't
need another |aFrame->ReflowDirtyChild|. Moreover, reflow commands are
coalesced, see |PresShell::AppendReflowCommand|. Hence, those multiple mouse
movements result in only one reflow (more or less). And so you don't need a
timer there. Imagine having a timer at every callsite, the codebase will be a
nightmare... (The reason you needed a timer was because of the wrong
|aFrame->ReflowDirtyChild| that you added.

I suggest you check if the Style System is effectively returning
|nsChangeHint_ReflowFrame| and that |nsCSSFrameConstructor::StyleChangeReflow|
is effectively called on the frame. From there on, you could follow from comment
13 above.

Comment 19

[Hideo Saito]

Attachment: patch

Comment 20

[Hideo Saito]

Attachment: testcase #2

'flow' can not move by mouseover.

Comment 21

[Hideo Saito]

For the testcase #2, 'flow' can not move. This problem is a regression from Bug
20022, especially the following changes, because a event of the mouseout occures
immediately a event of the mouseover occures.

 void
 PresShell::DidDoReflow()
 {
   HandlePostedDOMEvents();
   HandlePostedAttributeChanges();
   HandlePostedReflowCallbacks();
+  // Null-check mViewManager in case this happens during Destroy.  See
+  // bugs 244435 and 238546.
+  if (!mPaintingSuppressed && mViewManager)
+    mViewManager->SynthesizeMouseMove(PR_FALSE);
 }

I think that the event of mouseout should be delayed till the mouse moves when
the condition of the multiple mouseover/mouseout movements occures.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What is it you're trying to accomplish here, and why can't it be done by
tweaking nsViewManager::ProcessSynthMouseMoveEvent?

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And why are you special-casing font-size changes when many types of changes can
cause layout to change?

Comment 24

[Hideo Saito]

Attachment: patch

> What is it you're trying to accomplish here,

I want to do following two things.

1. For the testcase, to jump the text moved to the next line back to the
previous line.
2. To delay the event of mouseout till the mouse moves when layout of the
target frame changes.

> And why are you special-casing font-size changes when many types of changes
can
> cause layout to change?

It is wrong to see only font-size, as your indication.

With the latest patch, the difference in size is checked about both target
frame and parent frame, because a parent frame may change in size, even if a
target frame does not change.

Comment 25

[Hideo Saito]

Attachment: patch

This patch came from a patch(attachment 183583 [details] [diff] [review]) posted to Bug 189367. Please
refer to following testcase and screen shot. 
I think that the change of the line layout by occuring the reflow events causes
to mark the previous line dirty.

testcase for this patch: attachment 183584 [details]
screen shot testcase: attachment 183585 [details]

Comment 26

[Hideo Saito]

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

If the first element of the line is an inline frame, the previous line is
marked dirty to continue reflow. I think that the regression is a minimum, but
the cost increases.

Comment 27

[Hideo Saito]

Attachment: patch

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

nsBlockFrame::MarkLineDirty already has code to do something like this, and the
tests there are correct (whereas an inlineFrame frame type test is wrong).

At the very least this code should use that test; however, I'd think a better
fix would be to call MarkLineDirty instead of whatever MarkDirty call already
exists (though I didn't look at the surrounding code).

Comment 29

[Hideo Saito]

Attachment: patch

In changes, MarkLineDirty is directly called instead of MarkDirty. By the test,
previous lines are not marked dirty repeatedly.

Comment 30

[rbs]

Which is what I draw your attention to in comment 13 a good while ago.

Comment 31

[Hideo Saito]

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

This change effects incremental reflow, since the just affected line also
affects the previous line. I think that the cost is minimized.

Comment 32

[Hideo Saito]

dbaron, could you check in to the trunk?

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to trunk and 1.8branch.