135146 - eliminate BRS_ISINLINEINCRREFLOW in favor of reflow roots

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Chris Waterson]

dbaron has suggested that we ought to be able to improve typing performance and
fix several typing regressions by allowing reflow to start at a frame other than
the document's root frame.

Currently, when typing in a text field, the reflow starts at the document's root
frame, and proceeds down to target frame. The block code has some special
optimizations (e.g., BRS_ISINLINEINCRREFLOW and nsLineLayout::ReflowFrame) to
stifle damage propagation as reflow proceeds out of the text input frame.
Instead, we ought to simply start the reflow at the text input frame.

Comment 1

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

This will also allow removing a significant source of complexity in the block
code.  (Hopefully people won't start using 'display: inline-block' elements to
take up large areas when they meant blocks once CSS3 adds that, because then
we'd need that complexity back for letting incremental reflow into inlines again.)

Comment 2

[Chris Waterson]

Attachment: proposed fix

This adds a `NS_FRAME_REFLOW_ROOT' frame state bit that the
nsGfxTextControlFrame2 sets on its scroll frame during SetInitialChildList.
nsHTMLReflowCommand::BuildPath honors this bit by truncating the reflow path if
it encounters a frame with the bit set. nsHTMLReflowCommand::Dispatch was
modified so that a reflow that begins at a frame other than the root frame of
the frame hierarchy uses the `reflow root' frame's width and height as the
available space.

Comment 3

[Chris Waterson]

So the above patch doesn't actually get rid of the BRS_ISINLINEINCRREFLOW crap.
But it's good for flavor and fixes bug 110325 and bug 123623. I'm going to run
with the above patch for a while and see if I encounter any problems.

Comment 4

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

Since |GetContainingBlock| just does GetParent (it probably did something else
in the past, I'd guess), I think you can remove that comment about the floater
stuff as well as all the style->mFloat checks, since the two codepaths do
exactly the same thing now, and also GetContainingBlock itself.

Comment 5

[Chris Waterson]

Attachment: revised patch

Fix variable hiding problem kin pointed out; remove BRS_ISINLINEINCRREFLOW
stuff, remove nsHTMLReflowCommand::GetContainingBlock and related floater
monkey business.

Comment 6

[Chris Waterson]

Rolling the dice for mozilla-1.0. Reviews, anyone?

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 77771 [details] [diff] [review]
revised patch

I've been up to my ears in this code, and this looks great.  Are there any
other frames other than scrollframes for which setting the reflow root bit
makes sense?

r=[e-mail] (if people think I'm qualified to r= on layout changes).

Comment 8

[Chris Waterson]

Well, this only sets the NS_FRAME_REFLOW_ROOT bit on the scroll frame inside of
GFX text control frames (i.e., <input type="text"> and <textarea>). We could
conceivably set this bit on other scroll frames (e.g., those created by <select>
-- this may solve bug 119311, if bryner still cares).

Comment 9

[Asa Dotzler [:asa]]

adding this to the list of things we'd like to get for 1.0.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

I tried merging this fix with the latest reflow tree branch.  (Updated to tip,
not yet released to CVS).  

It definitely improves things in terms of painting text widgets on bugzilla bug
pages - the regression goes away entirely.

I am getting some assertions (and this may be due to my other changes in the
reflow tree branch!) from UnWind(), which could not get a parent successfully in
GetBoxForFrame() when typing into a text widget.  Also, another regression with
typing re-appeared (every other character typed doesn't echo immediately). 
Again, this may have nothing to do with this patch; it may be the reflow tree code.

Again, this only points out that we should have some people drop this into clean
trees and try it in debug builds.  I suspect the problems above are either
merging mistakes or issues with the base reflow tree code.

I definitely want this for 1.0 if possible.

Comment 11

[Chris Waterson]

rjesup: could you point me to the pages where you got assertions and regressions?

Comment 12

[Chase Tingley]

I've got the patch in my (trunk) tree, and I don't see the problems rjesup
mentioned.

Comment 13

[Marc Attinasi]

I like the patch, I'd just liek to see what happens if a text control is resized
via script, so I'll apply the patch and try it out for a while before SRing.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

I doubt my problems (when combined with the 129115 branch) are problems with
this patch; more likely they're interactions and/or imperfect merging on my part.

Comment 15

[Marc Attinasi]

Comment on attachment 77771 [details] [diff] [review]
revised patch

Kin showed me the testcase I was thinking of and also explained why it works
correctly - I like this. sr=attinasi

Comment 16

[kinmoz]

Comment on attachment 77771 [details] [diff] [review]
revised patch

r=[e-mail]

I did ask waterson to add some comments that note his findings about setting
the bit on inline frames.

I've been playing with these changes all morning and have yet to run across any
problems that can be attributed directly to it, so this is looking real
promising!

Comment 17

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

Comment on attachment 77771 [details] [diff] [review]
revised patch

This patch looks great to me.  Any idea how it helps with typing performance? 
(While I was home over spring break, I was using Mozilla on my old P-233
(Linux), and I found I could (when typing quickly) type faster into Bugzilla
textareas than Mozilla could keep up with me.)

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

It should help with typing according to waterson

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 77771 [details] [diff] [review]
revised patch

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 20

[Chris Waterson]

Checked in on the trunk. I'll wait a day or so to make sure there are no side
effects before landing on the mozilla-1.0 branch.

Comment 21

[kinmoz]

Yee haw ... adding bug 123058 and bug 136562 to list of bugs this blocks.

This bug doesn't actually fix the underlying cause of those bugs, but it 
definitely masks the problem and gets things working again because it causes 
incremental reflows to skip over the oh-so-troublesome-during-reflow-box-frame.

Comment 22

[Marc Attinasi]

Do you think that this will work OK in a dynamic way? For example, if an element
has a specific width and height (like a DIV or IMG with widht and height
specified) can we flick this new bit and expect the correct behavior? Then, if
those attrbutes / properties are changed, unflick the bit and possibly enqueue
another reflow? This could be a real winner for all kinds of situations where
the size cannot change (I'm thinking of IFRAMES too).

Comment 23

[Chris Waterson]

Checked in on mozilla-1.0 branch.

Comment 24

[Asa Dotzler [:asa]]

adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it
was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify
the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword
with verified1.0.0.

Comment 25

[Chris Petersen]

Marking verified on branch OS X (2002-05-22-05)