Last Comment Bug 92217 - [reflow] text doesn't rewrap after becoming small enough to wrap
: [reflow] text doesn't rewrap after becoming small enough to wrap
Status: RESOLVED FIXED
[awd:tbl]
: fixed1.8, testcase
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: P3 minor (vote)
: mozilla1.8beta5
Assigned To: Hideo Saito
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-07-24 22:15 PDT by Jesse Ruderman
Modified: 2005-09-26 03:44 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (407 bytes, text/html)
2001-07-24 22:16 PDT, Jesse Ruderman
no flags Details
patch (6.03 KB, patch)
2004-10-28 09:25 PDT, Hideo Saito
bernd_mozilla: review-
Details | Diff | Splinter Review
testcase for patch (1.93 KB, text/html)
2004-10-28 09:27 PDT, Hideo Saito
no flags Details
testcase to show that the patch is wrong (650 bytes, text/html)
2004-10-28 21:51 PDT, Bernd
no flags Details
patch (5.65 KB, patch)
2004-11-06 06:35 PST, Hideo Saito
dbaron: review-
Details | Diff | Splinter Review
testcase #2 (428 bytes, text/html)
2004-11-06 06:39 PST, Hideo Saito
no flags Details
patch (6.16 KB, patch)
2004-11-08 03:44 PST, Hideo Saito
no flags Details | Diff | Splinter Review
patch (2.02 KB, patch)
2005-06-11 17:35 PDT, Hideo Saito
no flags Details | Diff | Splinter Review
patch (1.18 KB, patch)
2005-07-07 08:21 PDT, Hideo Saito
dbaron: review-
Details | Diff | Splinter Review
patch (1.24 KB, patch)
2005-07-27 04:04 PDT, Hideo Saito
dbaron: review+
dbaron: superreview+
benjamin: approval1.8b4+
Details | Diff | Splinter Review

Description Jesse Ruderman 2001-07-24 22:15:49 PDT
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 2001-07-24 22:16:01 PDT
Created attachment 43496 [details]
testcase
Comment 2 Jesse Ruderman 2001-07-24 22:17:14 PDT
May be related to bug 76117, checkbox label doesn't unwrap when resizing browser
window.  76117 is not consistently reproducable.
Comment 3 anthonyd 2001-12-11 16:08:54 PST
the extra line problem seems to only happen with tables
Comment 4 karnaze (gone) 2002-01-07 09:36:18 PST
Temporarily moving to future until a milestone can be assigned. 
Comment 5 karnaze (gone) 2003-03-31 11:11:07 PST
mass reassign to default owner
Comment 6 Hideo Saito 2004-10-13 19:06:39 PDT
All/Alll
Comment 7 Hideo Saito 2004-10-28 09:25:13 PDT
Created attachment 163710 [details] [diff] [review]
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 2004-10-28 09:27:57 PDT
Created attachment 163711 [details]
testcase for patch
Comment 9 Hideo Saito 2004-10-28 09:35:22 PDT
> 1. PRUint32 mEventTime;
1. PRTime mEventTime;
Comment 10 Bernd 2004-10-28 21:50:57 PDT
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 2004-10-28 21:51:38 PDT
Created attachment 163783 [details]
testcase to show that the patch is wrong
Comment 12 Bernd 2004-10-28 21:53:21 PDT
this is a line issue
Comment 13 rbs 2004-10-28 22:11:03 PDT
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 2004-10-29 00:48:36 PDT
(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 2004-10-29 01:06:55 PDT
(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 2004-10-29 01:25:28 PDT
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 2004-10-29 09:17:18 PDT
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 2004-10-29 19:20:29 PDT
> 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 2004-11-06 06:35:17 PST
Created attachment 164858 [details] [diff] [review]
patch
Comment 20 Hideo Saito 2004-11-06 06:39:06 PST
Created attachment 164859 [details]
testcase #2

'flow' can not move by mouseover.
Comment 21 Hideo Saito 2004-11-06 06:41:44 PST
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-10 2004-11-06 17:58:26 PST
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-10 2004-11-06 18:00:09 PST
And why are you special-casing font-size changes when many types of changes can
cause layout to change?
Comment 24 Hideo Saito 2004-11-08 03:44:46 PST
Created attachment 165118 [details] [diff] [review]
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 2005-06-11 17:35:42 PDT
Created attachment 185997 [details] [diff] [review]
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 2005-06-21 18:04:01 PDT
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 2005-07-07 08:21:10 PDT
Created attachment 188540 [details] [diff] [review]
patch
Comment 28 David Baron :dbaron: ⌚️UTC-10 2005-07-25 14:39:20 PDT
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 2005-07-27 04:04:15 PDT
Created attachment 190689 [details] [diff] [review]
patch

In changes, MarkLineDirty is directly called instead of MarkDirty. By the test,
previous lines are not marked dirty repeatedly.
Comment 30 rbs 2005-07-27 19:48:09 PDT
Which is what I draw your attention to in comment 13 a good while ago.
Comment 31 Hideo Saito 2005-07-29 20:03:17 PDT
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 2005-08-02 19:11:38 PDT
dbaron, could you check in to the trunk?
Comment 33 Masayuki Nakano [:masayuki] (Mozilla Japan) 2005-09-26 03:41:43 PDT
checked-in to trunk and 1.8branch.

Note You need to log in before you can comment on or make changes to this bug.