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

RESOLVED FIXED in mozilla1.8beta5

Status

()

Core
Layout: Block and Inline
P3
minor
RESOLVED FIXED
16 years ago
12 years ago

People

(Reporter: Jesse Ruderman, Assigned: Hideo Saito)

Tracking

({fixed1.8, testcase})

Trunk
mozilla1.8beta5
fixed1.8, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [awd:tbl])

Attachments

(5 attachments, 5 obsolete attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Comment 1

16 years ago
Created attachment 43496 [details]
testcase
(Reporter)

Comment 2

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

Comment 3

16 years ago
the extra line problem seems to only happen with tables
Whiteboard: [awd:tbl]

Comment 4

16 years ago
Temporarily moving to future until a milestone can be assigned. 
Status: NEW → ASSIGNED
Target Milestone: --- → Future

Comment 5

15 years ago
mass reassign to default owner
Assignee: karnaze → table
Status: ASSIGNED → NEW
Component: Layout → Layout: Tables
QA Contact: petersen → madhur
Target Milestone: Future → ---

Updated

15 years ago
Priority: -- → P3
Target Milestone: --- → Future

Updated

13 years ago
Keywords: testcase
(Assignee)

Comment 6

13 years ago
All/Alll
OS: Windows NT → All
Hardware: PC → All
(Assignee)

Comment 7

13 years ago
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.
(Assignee)

Comment 8

13 years ago
Created attachment 163711 [details]
testcase for patch
(Assignee)

Comment 9

13 years ago
> 1. PRUint32 mEventTime;
1. PRTime mEventTime;
(Assignee)

Updated

13 years ago
Attachment #163710 - Flags: review?(rbs)

Comment 10

13 years ago
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.
Attachment #163710 - Flags: review?(rbs) → review-

Comment 11

13 years ago
Created attachment 163783 [details]
testcase to show that the patch is wrong

Comment 12

13 years ago
this is a line issue
Assignee: core.layout.tables → nobody
Component: Layout: Tables → Layout: Block and Inline
QA Contact: madhur → core.layout.block-and-inline

Comment 13

13 years ago
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.
(Assignee)

Comment 14

13 years ago
(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);
+          }
(Assignee)

Comment 15

13 years ago
(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.


(Assignee)

Comment 16

13 years ago
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 ...
(Assignee)

Comment 17

13 years ago
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

13 years ago
> 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.
(Assignee)

Comment 19

13 years ago
Created attachment 164858 [details] [diff] [review]
patch
Attachment #163710 - Attachment is obsolete: true
(Assignee)

Comment 20

13 years ago
Created attachment 164859 [details]
testcase #2

'flow' can not move by mouseover.
(Assignee)

Comment 21

13 years ago
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.
(Assignee)

Updated

13 years ago
Attachment #164858 - Flags: review?(dbaron)
What is it you're trying to accomplish here, and why can't it be done by
tweaking nsViewManager::ProcessSynthMouseMoveEvent?
And why are you special-casing font-size changes when many types of changes can
cause layout to change?
Attachment #164858 - Flags: review?(dbaron) → review-
(Assignee)

Comment 24

13 years ago
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.
Attachment #164858 - Attachment is obsolete: true
(Assignee)

Comment 25

12 years ago
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]
(Assignee)

Updated

12 years ago
Attachment #165118 - Attachment is obsolete: true
(Assignee)

Comment 26

12 years ago
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.
Attachment #185997 - Flags: review?(dbaron)
(Assignee)

Updated

12 years ago
Attachment #185997 - Attachment is obsolete: true
Attachment #185997 - Flags: review?(dbaron)
(Assignee)

Comment 27

12 years ago
Created attachment 188540 [details] [diff] [review]
patch
Attachment #188540 - Flags: review?(dbaron)
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).
Attachment #188540 - Flags: review?(dbaron) → review-
(Assignee)

Comment 29

12 years ago
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.
Attachment #188540 - Attachment is obsolete: true
Attachment #190689 - Flags: review?(dbaron)

Comment 30

12 years ago
Which is what I draw your attention to in comment 13 a good while ago.
Attachment #190689 - Flags: review?(dbaron) → review+
(Assignee)

Updated

12 years ago
Attachment #190689 - Flags: superreview?(dbaron)
Attachment #190689 - Flags: superreview?(dbaron) → superreview+
(Assignee)

Comment 31

12 years ago
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.
Attachment #190689 - Flags: approval1.8b4?

Updated

12 years ago
Attachment #190689 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 32

12 years ago
dbaron, could you check in to the trunk?
Assignee: nobody → saito
checked-in to trunk and 1.8branch.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.8beta5
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.