299065 - Bidi resolution needs to split inlines in addition to text frames

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

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

Bidi resolution needs to split inline frames in addition to text frames, so that
inline frames have continuations when they change embedding levels.  This is
necessary to get inline backgrounds and borders correct on inline frames as well
as fix bugs that rely on knowing the embedding level of an inline frame (like
bug 299063).

Steps to reproduce: load attached testcase

Expected results:  blue letters are on blue backgrounds; olive letters are on
yellow backgrounds

Actual results: one pair of blue letters is on a yellow background.

Comment 1

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

Attachment: testcase

Comment 2

[Simon Montagu :smontagu]

I'm not sure if this is the same issue as
http://smontagu.org/testcases/interlockinglinks.html, but I don't think I ever
filed a bug on that anyway.

Comment 3

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

I don't think so -- that doesn't even look like it demonstrates any bugs, unless
you think some of those elements should have 'unicode-bidi: embed' in the UA
stylesheet.

Comment 4

[Simon Montagu :smontagu]

I clearly need to rewrite that testcase to make it clearer what's going on. The
bugs are exhibited when hovering over the elements. For example, when hovering
on the blue word משפט in the second line, I see the title text "This is the
green text".

Comment 5

[Simon Montagu :smontagu]

Sorry, I meant the blue word משפט

Comment 6

[Uri Bernstein]

I'm pretty sure doing this will fix bug 178991.

Comment 7

[Uri Bernstein]

Attachment: WIP 1

So, I decided to take a stab at this.
The attached patch is WIP. I'd like to get some feedback from smontagu and dbaron so that I know if I'm at all on the right track.

The attached patch fixes the testcase attached to this bug, as well as bug 178991 and Simon's testcase from comment #2.
Work obviously remaining to be done: destroy the inline continuation frames in RemoveBidiContinuation() (this is not trivial because children need to be returned to their original parents).

Some functional problems with this patch:
- When an inline has left/right borders, those borders now appear whenever the inline is split (i.e. wherever the direction changes). David - is there anything simple I can do to fix this, or should this be handled as part of bug 299063?
- Caret movement (in HTML with split inlines) is screwed up. I'm not terribly worried about this at this stage, as caret movement in Bidi HTML is screwed up anyhow (see bug 228290), and splitting the inlines should, eventually, make fixing these problems easier.
- When I open a page with split inlines in composer, only the first part is styled. I haven't really investigated this yet.

Questions:
- Currently I'm not creating any special relationship between the "new" inlines and the ones they were split off (i.e., I'm not setting Next/PrevInFlow, or the nsLayoutAtoms::nextBidi attribute). Should I set any of these? Or something else?

Comment 8

[Simon Montagu :smontagu]

I won't have time to look at this before the beginning of next week.

Comment 9

[Uri Bernstein]

(In reply to comment #7)

> - Currently I'm not creating any special relationship between the "new" inlines
> and the ones they were split off (i.e., I'm not setting Next/PrevInFlow, or the
> nsLayoutAtoms::nextBidi attribute). Should I set any of these? Or something
> else?

Obviously I have to do something, because with this patch, dynamic changes made to the style of the inline element only affect the first (original) frame. I suspect this is also the reason for the problem I'm seeing with composer.

Comment 10

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

comment 7 is correct:  you definitely need to do something.  You probably can't just use next-in-flow directly without adding another bit for fixed-next-in-flow to indicate that it's a next-in-flow that's not re-wrappable.  You might be able to get nextBidi to work, although you'd probably need to add nextBidi checks in a bunch of additional places that currently check only for next-in-flow.

I think I prefer the former approach, but it's probably best done in steps for ease of reviewing, i.e.:
 (1) make functions GetNextWrappableInFlow / GetPrevWrappableInFlow on nsIFrame that just call GetNextInFlow/GetPrevInFlow and convert the things that do wrapping of next-in-flows/prev-in-flows to use those.  (Actually, I don't like those names.  Maybe it would be better to use GetNextInFlow/GetPrevInFlow for the "wrappable" meaning and GetNextContinuation/GetPrevContinuation for any continuation, wrappable or not.  Although I suspect you'd end up with more renaming that way.)
 (2) convert the existing nextBidi on text frames to use a bit that says it's not wrappable, and change the functions that are supposed to return only wrappable continuations to check that bit
 (3) add splitting of inline frames

It would probably be good to hear what roc and bz think about this before starting, though, since they could easily disagree.

Comment 11

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

I don't know much about the bidi code, but here's what I think are the basic concepts we're trying to capture:
1) a doubly-linked list of frames that together represent the rendering of an element
2) between each pair of consecutive frames in the list there is either a "hard" division or a "soft" division (not-wrappable or wrappable in David's terms). The latter divisions can be deleted or created during reflow and represent soft line, column and page breaks; the former divisions cannot be created or deleted during reflow and represent divisions based on the underlying content.

Equivalently we could think of the major doubly-linked list as being a sequence of sublists separated by "hard" divisions. I think it's easier just to classify the divisions.
It seems to me that "hard divisions" could be used to handle not just bidi frame splitting, but also special siblings for IB splitting, and maybe also newlines in 'white-space:pre', and maybe also first-letter frame splitting. These are all really the same thing, perhaps.

If we all agree with that, then I agree with David's plan, but I would add extra steps at the end: convert IB special siblings to use the new mechanism, followed by the other situations if they really can be handled the same way.

So GetPrevInFlow/GetNextInFlow would remain and continue to traverse the complete list. I can't think of a great name for the new functions either. How about GetPrevBeforeSoftBreak/GetNextAfterSoftBreak? Hmm, that's not clearly better than *WrappableInFlow, to me...

Comment 12

[Boris Zbarsky [:bzbarsky]]

I think the idea of having continuations tagged by type of continuation is a good one; if nothing else, it ought to make bidi continuations a reasonable part of the overall layout model.

As far as naming goes, using GetNextInFlow/GetPrevInFlow for its current meaning (fluid continuations) and using GetNextContinuation/GetPrevContinuation for the new continuation setup seems like pretty good naming to me.  It does require more renaming, but the original rename could happen in one step of pure search/replace before we do step 1 from comment 10.

That said, for wrapping we might want more than just GetNextInFlow/GetPrevInFlow, since what we'll really be interested in, I suspect is the next location where stuff could be flown.  That is, we'd need to get out both the frame before and the frame after the flowable link, right?

Comment 13

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

(In reply to comment #12)
> That said, for wrapping we might want more than just
> GetNextInFlow/GetPrevInFlow, since what we'll really be interested in, I
> suspect is the next location where stuff could be flown.  That is, we'd need to
> get out both the frame before and the frame after the flowable link, right?

I'm not sure what you're getting at here.

Comment 14

[Boris Zbarsky [:bzbarsky]]

What I mean is that we're basically talking about being able to go from a frame to the nearest place where we can pull things up from the next line or something.  This requires finding two things -- the first continuation that's on the next line, and the last continuation that's on the current line (here I'm including bidi stuff in "continuation").  I guess we can just GetNextInFlow() to find the former and then call GetPrevContinuation on it to get the latter...

Comment 15

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

(In reply to comment #11)
> If we all agree with that, then I agree with David's plan, but I would add
> extra steps at the end: convert IB special siblings to use the new mechanism,
> followed by the other situations if they really can be handled the same way.

Yeah, I've thought about using this for other things during some of the discussions about inline layout and the reflow branch.

But some of the other cases break invariants that may be useful to maintain:
 * IB special siblings are different frame types (this is a pretty big deal; I'm quite hesitant about breaking it)
 * both IB special siblings and continuations across :first-letter/:first-line  (do we already use n-i-f for these?) have different style data

I think there may have been another case that didn't break either of these invariants.

Comment 16

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

(I even proposed it in bug 296083 comment 4, although I've become a bit more hesitant since.)

Comment 17

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

(In reply to comment #14)
> What I mean is that we're basically talking about being able to go from a
> frame to the nearest place where we can pull things up from the next line or
> something.  This requires finding two things -- the first continuation that's
> on the next line, and the last continuation that's on the current line (here
> I'm including bidi stuff in "continuation").  I guess we can just
> GetNextInFlow() to find the former and then call GetPrevContinuation on it to
> get the latter...

Two different things happen with pull-up.
1) Pulling up an entire frame into the current line from the next line.
http://lxr.mozilla.org/mozilla/source/layout/generic/nsBlockFrame.cpp#3787
2) Extending the current frame to map more content, at the expense of its next-in-flow(s), by moving some of a later-in-flow's children to the current frame or (for text frames) simply adjusting content offsets in the current frame and its next-in-flow(s).
Note that if we had frames for lines then 1) would be a special case of 2).

Step 2) should not be able to pull across hard divisions/fixed continuation boundaries. So, using your terminology, implementations of step 2) will call GetNextInFlow only as they search for next-in-flows to pull from.

Step 1), however, can pull up any sort of continuation, it just chooses the next frame on the block's child list and doesn't actually call GetNextInFlow/GetNextContinuation at all. Step 1) only happens when the current line is a line of inlines, so it checks that the frame it's pullling up is not block-level; see
http://lxr.mozilla.org/mozilla/source/layout/generic/nsBlockFrame.cpp#2869

There's a similar "pull-down" process, not used for lines, but used for block and column continuations, where we pull overflowing child frames from a prev-in-flow block or column into the curernt block or column. This just needs to continue using GetPrevInFlow.

So where would GetNextContinuation be called? Presumably in the frame constructor when we need to restyle or remove frames for an element (where we already traverse IB special siblings). Presumably in the places where we need to calculate the bounding box of the frames for an element. Perhaps in some places in the caret/selection code. Where else?

> * IB special siblings are different frame types (this is a pretty big deal;
> I'm quite hesitant about breaking it)

We definitely need this to remain true for GetNext/PrevInFlow, but I don't know that we need it for GetNext/PrevContinuation.

> * both IB special siblings and continuations across :first-letter/:first-line 
> (do we already use n-i-f for these?) have different style data

Ditto.
Seems to me the right way to go is to get this working for bidi frames first and then try porting the other cases to the same mechanism, one at a time.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> So where would GetNextContinuation be called?

In addition to the places you listed, from ReResolveStyleContext and ReParentStyleContext (and this is where having different continuations have different style contexts could be problematic).  That's all I can think of at the moment, though.

I agree that we should do bidi and then see how things look.

Comment 19

[Uri Bernstein]

Attachment: WIP 2

This implements the "fluid" vs. "non-fluid" distinction for continuations, as discussed in previous comments.
This patch WFM with some testcases I have, and also works (or at least dosn't crash) with a few real-world complex Hebrew sites.
The "functional problems" I mentioned in comment #7 are all fixed.

As you can see, a lot of this patch is just converting places which accessed mNext[Prev]InFlow directly to use GetNext[Prev]InFlow().
I changed places to use GetNext[Prev]Continuation() mainly where I needed to do so to get my testcases to work. I'm not sure I can map all of the cases mentioned in comments #17 and #18 to actual places in the code - I might need some help with that. (The code contains >100 calls to GetNextInFlow() - ideally someone should go over all of them and see which should be changed and which shouldn't).

Question:
- Should I try and make nsSplittableFrame::RemoveFromFlow() shorter/more efficient? Should I rename it, now that it handles non-fluid continuations as well?
- Does nsSplittableFrame::BreakFromFlow() have to handle non-fluid continuations?

I still haven't taken care of RemoveBidiContinuation(), which is the main reason this patch is not ready for review. However, if anyone cares to take a look and comment I'll be very thankful.

Comment 20

[Uri Bernstein]

Attachment: WIP 3

This should have been ready for review, except that I can easily trigger a crash/hang with this, so I'll have to investigate that further.

Highlights in this patch:
- Got rid of the nsLayoutAtoms::nextBidi attribute.
- Answered my own question regarding BreakFromFlow().
- Finally did RemoveBidiContinuation() - this is probably the cause of the crashes I'm seeing

Comment 21

[Uri Bernstein]

Comment on attachment 207486 [details] [diff] [review]
WIP 3

It turns out that the logic in nsBidiPresUtils::Resolve() is more complex than I originally assumed, and making it work with split inlines would require some careful redesign.
Also, unfortunately, I currently have much less free time to work on Mozilla.
I still hope to continue working on, and eventually finish, this, but it will take longer than I thought.

Comment 22

[Uri Bernstein]

Attachment: patch v1

This is hopefully ready for review.

RemoveBidiContinuation() does not actually remove inline continuations - it just converts them to fluid continuations, which the inline reflow algorithm can then re-use or delete. Similarly, EnsureBidiContinuation() can reuse any continuation (fluid or not) as a bidi continuation. When reusing an existing continuation, it now ensures that its inline parents become non-fluid continuations).
This is a "conservative" approach - it's the most similar to what happened before. I tried some other ideas, with no success.

I'm requesting review from dbaron, but I'd appreciate smontagu's input as well.

Comment 23

[Uri Bernstein]

Comment on attachment 208504 [details] [diff] [review]
patch v1

in nsContainerFrame::List(), |if (nsnull != GetPrevInFlow())| and |if (nsnull != GetNextInFlow())| should use GetPrevContinuation() and GetNextContinuation(), of course. I'll fix this in the next version of the patch.

Comment 24

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

I can review this if that's OK with dbaron.

Comment 25

[Uri Bernstein]

Splitting inline frames ensures that each of a line's direct children has a single bidi embedding level. This allows us to greatly simplify what nsBidiPresUtils::ReorderFrames() does: it now only has to reorder the direct children of the given line, not the leaves. I tried this, and I was happy to find out that it also fixes bug 237370 and bug 258928.

I'm not sure if I should include this change with the patch on the current bug, or do it as a follow-up (I already feel like I have too many balls up in the air here).

Comment 26

[Uri Bernstein]

A testcase provided by smontagu alerted me to the fact that I'm currently not splitting inlines in the case where no text frame needs to be split (e.g. if an inline has two inline container children, each containing unidirectional text, in opposite directions).

So I'll have to fix that and post an updated patch. However, everything outside nsBidiPresUtils still stands for review, if someone (dbaron or roc) wants to have a look.

Comment 27

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

FWIW, I'm a bit behind on my review queue lately; It'll probably be a few weeks before I get to this, although I'm hoping to do some catching up soon...

Comment 28

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

Comment on attachment 208504 [details] [diff] [review]
patch v1

>+  // Continuation member functions.
>+  virtual nsIFrame* GetPrevContinuation() const;
>+  NS_IMETHOD  SetPrevContinuation(nsIFrame*);
>+  virtual nsIFrame*  GetNextContinuation() const;
>+  NS_IMETHOD  SetNextContinuation(nsIFrame*);
>+
>   // Flow member functions.
>   virtual nsIFrame* GetPrevInFlow() const;
>   NS_IMETHOD  SetPrevInFlow(nsIFrame*);
>   virtual nsIFrame*  GetNextInFlow() const;
>   NS_IMETHOD  SetNextInFlow(nsIFrame*);

This header definitely needs some more documentation.  There should probably be a short description at the beginning about the difference between continuations and in-flows (one being a subset of the other), and then you need to explain the semantics of the 8 functions (although the next/prev pairs should be pretty similar).

Comment 29

[Uri Bernstein]

Attachment: patch v2

Many changes and fixes (sorry, I really thought v1 was ready for review):
1. Included the changes mentioned in comment #25.
2. Fixed the issue mentioned in comment #26 (splitting ancestor inlines even when text inlines are not split). Actually, I now split ancestor inlines between every two leaves, even if they have the same bidi level. This is somewhat wasteful, but it is necessary for the comment #25 changes to work properly, and Simon agreed with me that this is a fair price to pay in order to fix the two bugs mentioned there.
3. Prevent borders, paddings, and inlines from showing up between non-fluid continuations. (the borders themselves were already skipped in the previous patch, but they were still taking horizontal space). Thanks to dbaron for helping with this.
4. Added comments to nsSplittableFrame.h as per comment #28.
5. Attempt to handle outlines correctly. Outlines should be skipped between non-fluid continuations, but not between fluid continuations. In practice, in RTL text, this doesn't work very well, due to what is essentially bug 299063 (the outlines' like borders, are oftent rendered in the wrong places).

Comment 30

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

(In reply to comment #29)
> 5. Attempt to handle outlines correctly. Outlines should be skipped between
> non-fluid continuations, but not between fluid continuations. In practice, in
> RTL text, this doesn't work very well, due to what is essentially bug 299063
> (the outlines' like borders, are oftent rendered in the wrong places).

I think we should simply have outlines drawn around every single frame and then rely on to-be-written outline drawing logic to consolidate the individual outline rects into a single shape that outlines the union of the rects. (This will become relatively easy to do after frame display lists land.) Then we won't need GetSkipOutlineSides, so I suggest you don't bother with it here.

Comment 31

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

I'm a bit concerned about the codesize/perf impact of converting lots of uses of mNextInFlow/mPrevInFlow into virtual calls.

If C++ had 'final', we could make nsSplittableFrame::GetNext/PrevInFlow final, but it doesn't. However, since we really only have two implementations of nsIFrame::GetNext/PrevInFlow, there is a simple workaround:

In nsIFrame:
  virtual nsIFrame* GetNextInFlowVirtual() = 0;
  nsIFrame* GetNextInFlow() { return GetNextInFlowVirtual(); }

In nsSplittableFrame:
  virtual nsIFrame* GetNextInFlowVirtual() { return GetNextInFlow(); }
  nsIFrame* GetNextInFlow() {
    return mNextContinuation &&
      (mNextContinuation->GetStateBits() & NS_FRAME_IS_FLUID_CONTINUATION)
      ? mNextContinuation : nsnull;
  }

similarly in nsTextFrame.

Then we're good to go.

Comment 32

[Uri Bernstein]

Attachment: patch v2.1

Removed the outline stuff per comment #30, and did the funky C++ stuff suggested in comment #31.

I'm requesting review from dbaron again - but roc, if you can review this and dbaron doesn't mind, I don't mind either, of course.

Comment 33

[Uri Bernstein]

Attachment: patch v3

Sorry for making this a moving target.

Changes from v2.1 are only in nsBidiPresUtils. I no longer split between same-direction leaves (see comment #29 point 2). Instead, in RepositionInlineFrames() I drill down into RTL frames and simply reverse them.

I also removed the (very complex) code related to frames starting with diacritics (bug 40882), because that code actually made the testcase on that bug, as well as the more elaborate testcase in http://www.qsm.co.il/Hebrew/HebrewTest/ColorHtml.htm *not* work for me. This might be a platform-dependant issue, but I suspect that the fact that it breaks Mac indicates that it wasn't the correct solution in the first place.

Comment 34

[Uri Bernstein]

With patch v3, empty inlines (e.g. <span></span>) can screw up ordering. The solution is to treat such frames as leaf frames in nsBidiPresUtils::InitLogicalArray() (because that's what they really are).
I'll include this simple fix with the next version of the patch.

Comment 35

[Haamed Gheibi]

Our patch https://bugzilla.mozilla.org/attachment.cgi?id=209221 has problem with empty spans too. w3c does not specify that how empty spans should be treat in reoerdering.  A solution is that we treat them like they have a nutral element inside.

Comment 36

[Haamed Gheibi]

The actual size and style (e.g. paddings and borders) of splited frames cannot be determined before reordering.
Since for example padding-left should be applied to the left most splited frame in VISUAL order.
In your patches all of these styles are considered before reordering.

(nsBidiPresUtils.h is missed from the patch)

Comment 37

[Uri Bernstein]

Attachment: patch v3.1

I spent the last two days debugging a hard-to-reproduce incremental reflow bug (which eventually led to a crash). The problem turned out to be in nsCSSFrameConstructor::ContentAppended(), which used GetLastInFlow(), where it should have been finding the last continuation instead. I added Get(First|Last)Continuation() methods, and switched ContentAppended(), as well as two more places in nsCSSFrameConstructor, to use GetLastContinuation() instead of GetLastInFlow() (these are places which stepped over special frames, so I assume they really are after the last continuation, not just the last in flow).
I'm still worried that there might be other places which need to be changed to use (Next|Prev|Last|First)Continuation instead of ...InFlow. It's difficult for me to judge what these places are.

This patch also includes a fix for the problem mentioned in comment #34, and a fix to Reorder() which affected the layout of RTL drop-down selects (basically, when drilling down in to frames in nsBidiPresUtils, everyone now uses the same logic to determine which frames are "descendable").

Re comment #35: Yes, I am aware that this frame ordering method does not implement the current W3C standard. It also does not take care of margins. I suppose the final work on this will be done in bug 299063 and/or bug 121633. For now, this is just a simple fix which is much better than the previous situation.

Comment 38

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

Comment on attachment 209595 [details] [diff] [review]
patch v3.1

Some preliminary comments from skimming the patch:

nsSplittableFrame::RemoveFromFlow could be simplified -- instead of
calling methods it could be written simply in terms of unsetting the
fluid continuation bit on the next sibling iff there is a
prev-continuation and the frame being removed does not have the bit set.

You shouldn't need to pass aFrame to nsLineLayout::CanPlaceFrame since
it's already pfd->mFrame (right?).

In SplitInlineAncestors in nsBidiPresUtils.cpp, when you check the frame
type, you need to check both nsLayoutAtoms::inlineFrame and
nsLayoutAtoms::positionedInlineFrame.  Likewise, you should use
nsCSSFrameConstructor::CreateContinuingFrame to create the right type of
continuing frame -- it will also handle a good bit of the other work
that happens in that function.

I'm not convinced that using a magical nsLayoutAtoms::nextBidi parameter
is the best way to communicate between SplitInlineAncestors and
nsInlineFrame::AppendFrames.  Perhaps you could do something more like
nsInlineFrame::PushFrames.  Manipulating frame lists without using the
AppendFrames/InsertFrames methods is often the better way to go --
there's not much work to maintaining a singly-linked list.

Comment 39

[Uri Bernstein]

(In reply to comment #38)
> nsSplittableFrame::RemoveFromFlow could be simplified -- instead of
> calling methods it could be written simply in terms of unsetting the
> fluid continuation bit on the next sibling iff there is a
> prev-continuation and the frame being removed does not have the bit set.

Will do (although not using the methods means I have to cast into nsSplittableFrame*, which I was trying to avoid).
BTW, the method name should probably be changed as well (as it is removing the frame from the "continuation chain", not necessarily the "flow"). Is "continuation chain" a good term? We need an equivalent of "flow" for not-neccessarily-fluid continuations.

> You shouldn't need to pass aFrame to nsLineLayout::CanPlaceFrame since
> it's already pfd->mFrame (right?).

Right. I didn't notice that. Will fix.

> In SplitInlineAncestors in nsBidiPresUtils.cpp, when you check the frame
> type, you need to check both nsLayoutAtoms::inlineFrame and
> nsLayoutAtoms::positionedInlineFrame.

Will do, also in nsBidiPresUtils::EnsureBidiContinuation when re-using existing continuations, and in nsBidiPresUtils::Reflow, when going up the tree to find the place SplitInlineAncestors should start splitting from.

> Likewise, you should use
> nsCSSFrameConstructor::CreateContinuingFrame to create the right type of
> continuing frame -- it will also handle a good bit of the other work
> that happens in that function.

nsCSSFrameConstructor::CreateContinuingFrame currently inserts the newly-created frame into the flow (by calling SetNext/PrevInFlow directly, and by calling Init(), which, for inline frames, ends up in nsSplittableFrame::Init, which again uses SetNext/PrevInFlow). Since in my case I'm looking to create a non-fluid continuation, should I change CreateContinuingFrame and nsFrame::Init to receive a boolean indicating whether this is a fluid continuation, or is there a simpler/better way?

> I'm not convinced that using a magical nsLayoutAtoms::nextBidi parameter
> is the best way to communicate between SplitInlineAncestors and
> nsInlineFrame::AppendFrames.

I was basically imitating CreateBidiContinuation() which does something similar. Should I change that as well? Also, just to be sure: you're suggesting that I manipulate the frame list directly, not that I use PushFrames(), right?

Comment 40

[Uri Bernstein]

(In reply to comment #39)
> (In reply to comment #38)
> > nsSplittableFrame::RemoveFromFlow could be simplified -- instead of
> > calling methods it could be written simply in terms of unsetting the
> > fluid continuation bit on the next sibling iff there is a
> > prev-continuation and the frame being removed does not have the bit set.
> 
> Will do (although not using the methods means I have to cast into
> nsSplittableFrame*, which I was trying to avoid).

Actually, no. I can't cast, because RemoveFromFlow is also used with ns(Continuing)TextFrame, which does not inherit from nsSplittableFrame. So I don't really see how I can do what you're suggesting.

Comment 41

[Uri Bernstein]

Attachment: patch v3.2

Updated to (yesterday's) head; better implementations of nsFrameManager::InsertFrames and nsFrameManager::RemoveFrame; removed unused arguments of nsBidiPresUtils::ReorderFrames and nsBidiPresUtils::Reorder; addressed those of dbaron's comments which I didn't have questions about.

Comment 42

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

(In reply to comment #40)
> Actually, no. I can't cast, because RemoveFromFlow is also used with
> ns(Continuing)TextFrame, which does not inherit from nsSplittableFrame. So I
> don't really see how I can do what you're suggesting.

If it doesn't inherit from nsSplittableFrame, how does nsSplittableFrame's implementation matter?

Comment 43

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

Also, I'm thinking we probably should have some assertions in SetNextInFlow (etc., all 4 functions) that assert that the frames are the same frame type.  I think that's an invariant we do want to maintain.

Comment 44

[Uri Bernstein]

(In reply to comment #42)
> (In reply to comment #40)
> > Actually, no. I can't cast, because RemoveFromFlow is also used with
> > ns(Continuing)TextFrame, which does not inherit from nsSplittableFrame. So I
> > don't really see how I can do what you're suggesting.
> 
> If it doesn't inherit from nsSplittableFrame, how does nsSplittableFrame's
> implementation matter?
 
nsSplittableFrame::RemoveFromFlow() is a static method (so it's not really "nsSplittableFrame's implementation"). The frame itself is passed in as an IFrame* argument, and in practice can be either an nsSplittableFrame or an nsTextFrame.

Comment 45

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

> nsSplittableFrame::RemoveFromFlow() is a static method (so it's not
> really "nsSplittableFrame's implementation"). The frame itself is passed
> in as an IFrame* argument, and in practice can be either an
> nsSplittableFrame or an nsTextFrame.

OK, makes sense.

In nsSplittableFrame.h:

+   * Fluid continuations ("in-flows") are the result of line breaking.

line breaking, column breaking, or page breaking

IsDecsendable is misspelled: it's descend.  But I'm still not sure what
it means; it should probably have a clearer name.

Also, I'm thinking we probably should have some assertions in SetNextInFlow
(etc., all 4 functions) that assert that the frames are the same frame type.  I
think that's an invariant we do want to maintain.

I'd like to see the assertions I mentioned in comment 43.

It seems like your default search/replace in the table code was to use
GetNextInFlow()/GetPrevInFlow().  Since there's no distinction for table
frames, for code that's dealing with something other than the act of
page breaking, it seems like you could use
mNextContinuation/mPrevContinuation, at least in most of these places
(faster, too, since no virtual function call).

> nsCSSFrameConstructor::CreateContinuingFrame currently inserts the
> newly-created frame into the flow (by calling SetNext/PrevInFlow
> directly, and by calling Init(), which, for inline frames, ends up in
> nsSplittableFrame::Init, which again uses SetNext/PrevInFlow). Since
> in my case I'm looking to create a non-fluid continuation, should I
> change CreateContinuingFrame and nsFrame::Init to receive a boolean
> indicating whether this is a fluid continuation, or is there a
> simpler/better way?

Yes for CreateContinuingFrame; not sure about nsFrame::Init.  (You could
always just call Set*Continuation an extra time to work around that.  Or
clear NS_FRAME_IS_FLUID_CONTINUATION after calling Init.)

> I was basically imitating CreateBidiContinuation() which does
> something similar. Should I change that as well? Also, just to be
> sure: you're suggesting that I manipulate the frame list directly, not
> that I use PushFrames(), right?

Yes, manipulate the frame list directly.  If such a change seems like a
lot of additional work, feel free to push it off to a later patch, but I
think it is a better approach.

I haven't had a chance to give this really detailed review yet, but I'm
not sure it needs it.  There will probably be some places where the
continuation/n-i-f choice was made incorrectly, and I don't think review
will catch too many of those; I'm inclined to just r+sr once you've
addressed these issues, although I'm interested to know what roc thinks
about that or whether he wants to have a look.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Would it make sense to add "no loop" asserts to SetNext/PrevWhatever?  As in, assert that |this| is not on the continuation chain of the thing we're setting?

Comment 47

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

(In reply to comment #45)
> Yes for CreateContinuingFrame; not sure about nsFrame::Init.  (You could
> always just call Set*Continuation an extra time to work around that.  Or
> clear NS_FRAME_IS_FLUID_CONTINUATION after calling Init.)

Or, better, pass null to init and then connect afterwards, just like you do now (that's what you're doing now, right?).

Comment 48

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

You might want to add a comment somewhere in nsIFrame.h that we return NS_FRAME_COMPLETE when we have finished laying out a flow in the available space; the content might not yet be completely laid out, because there might still be continuations to be reflowed.

+    if ((NS_FRAME_IS_NOT_COMPLETE(aStatus) || (pfd->mFrame->GetNextContinuation() && !pfd->mFrame->GetNextInFlow())) 
+        && !pfd->GetFlag(PFD_ISLETTERFRAME)) {
       // Only apply end margin for the last-in-flow. Zero this out so

I think you could simplify the first line to just
+    if (pfd->mFrame->GetNextContinuation()
... right?

+  // Insert aFrameList after the last continuation of aPrevFrame.
   if (aPrevFrame) {
+    aPrevFrame = aPrevFrame->GetLastContinuation();
   }
   return aParentFrame->InsertFrames(aListName, aPrevFrame, aFrameList);

Seems like things could go horribly wrong if aPrevFrame->GetLastContinuation()'s parent is not aParentFrame. Should we reget the parent frame? Why do we even take aParentFrame as a parameter here???

+  // If a Bidi frame has a continuation frame, remove it before the first frame
+  nsIFrame* nextContinuation = aOldFrame->GetLastInFlow()->GetNextContinuation();
+  if (nextContinuation) {
+    RemoveFrame(aParentFrame, aListName, nextContinuation);

Ditto...

> Also, I'm thinking we probably should have some assertions in SetNextInFlow
> (etc., all 4 functions) that assert that the frames are the same frame type.

Yeah!

> it seems like you could use mNextContinuation/mPrevContinuation, at least in
> most of these places

I would prefer we didn't do that. I would leave the table code using GetNextInFlow(), which is what we really *mean* there ... using GetNextContinuation or mNextContinuation would be a subtle optimization, that might come undone later, or confuse the reader, IMHO of dubious value.

> I haven't had a chance to give this really detailed review yet, but I'm
> not sure it needs it.  There will probably be some places where the
> continuation/n-i-f choice was made incorrectly, and I don't think review
> will catch too many of those; I'm inclined to just r+sr once you've
> addressed these issues, although I'm interested to know what roc thinks
> about that or whether he wants to have a look.

I've looked through all the GetNextInFlow conversions and I didn't find any obvious errors, though I wouldn't be my life that there are none. I'm happy for this to land, if you've looked at the nsBidiPresUtils logic carefully.

Comment 49

[Uri Bernstein]

(In reply to comment #47)
> (In reply to comment #45)
> > Yes for CreateContinuingFrame; not sure about nsFrame::Init.  (You could
> > always just call Set*Continuation an extra time to work around that.  Or
> > clear NS_FRAME_IS_FLUID_CONTINUATION after calling Init.)
> 
> Or, better, pass null to init and then connect afterwards, just like you do now
> (that's what you're doing now, right?).
> 

Yes, that's what I'm doing, but looking closer there might be a problem with doing so, and there's almost certainly a problem with doing so in CreateContinuingFrame:
Some of the Init() methods (e.g. nsContainerFrame::Init() and nsTableCellFrame::Init()) do more with aPrevInFlow, other than passing it to nsSplittableFrame::Init(). If I pass null to these Init()s, there is no way for me to do the other actions afterwards.
So I think it's safer to go with your original suggestion.

Comment 50

[Uri Bernstein]

(In reply to comment #46)
> Would it make sense to add "no loop" asserts to SetNext/PrevWhatever?  As in,
> assert that |this| is not on the continuation chain of the thing we're setting?

I tried doing it the way you suggested. The problem is that often we do |f1->SetNextInFlow(f2); f2->SetPrevInFlow(f1);|, so by the time we get to the SetPrevInFlow, f2 is already (legitimately) on the continuation chain of f1. So perhaps I should instead assert that the thing we're setting is not on already on the continuation chain of |this|?
Another problem is that I now sometimes call SetNextContinuation with something which is already the nextInFlow, just to reset the bit (see previous comment). But this I can probably just special-case in the asserts.

Comment 51

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

(In reply to comment #50)
> I tried doing it the way you suggested. The problem is that often we do
> |f1->SetNextInFlow(f2); f2->SetPrevInFlow(f1);|, so by the time we get to the
> SetPrevInFlow, f2 is already (legitimately) on the continuation chain of f1. 

You should probably do these assertions by walking only in one direction, which is enough to check for loops in one set of the links:  SetNextInFlow should check the forward links starting from f2 to make sure f1 is not in the list, SetPrevInFlow should check the back links starting from f1 to make sure f2 is not in the list.  Asserting that the next/prev links are symmetric is a little harder, given this API.

Comment 52

[Boris Zbarsky [:bzbarsky]]

What dbaron said.  All I want is that if someone does f1->SetNextInFlow(f2) we assert that walking the GetNextContinuation() chaing of f2 does not reach f1.  For setting PrevInFlow, we walk the PrevContinuation chain.

Comment 53

[Uri Bernstein]

(In reply to comment #48)
> +    if ((NS_FRAME_IS_NOT_COMPLETE(aStatus) ||
> (pfd->mFrame->GetNextContinuation() && !pfd->mFrame->GetNextInFlow())) 
> +        && !pfd->GetFlag(PFD_ISLETTERFRAME)) {
>        // Only apply end margin for the last-in-flow. Zero this out so
> 
> I think you could simplify the first line to just
> +    if (pfd->mFrame->GetNextContinuation()
> ... right?

David, can you please help me answer this? This is basically the inverse of the condition I changed in nsInlineFrame::ReflowFrames() (for paddings and borders), which you suggested to me on IRC. I assume that if one of them can be simplified, then so can the other.

Comment 54

[Uri Bernstein]

(In reply to comment #52)
> What dbaron said.  All I want is that if someone does f1->SetNextInFlow(f2) we
> assert that walking the GetNextContinuation() chaing of f2 does not reach f1. 
> For setting PrevInFlow, we walk the PrevContinuation chain.

So I'm adding the following to nsSplittableFrame:

+  // Can aFrame2 be reached from aFrame1 by following prev/next continuations?
+  static PRBool IsInPrevContinuationChain(nsIFrame* aFrame1, nsIFrame* aFrame2);
+  static PRBool IsInNextContinuationChain(nsIFrame* aFrame1, nsIFrame* aFrame2);
  
and using them for the assertions in the variuos "set" methods. Sounds good?

Comment 55

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

(In reply to comment #54)
> So I'm adding the following to nsSplittableFrame:
> 
> +  // Can aFrame2 be reached from aFrame1 by following prev/next continuations?
> +  static PRBool IsInPrevContinuationChain(nsIFrame* aFrame1, nsIFrame*
> aFrame2);
> +  static PRBool IsInNextContinuationChain(nsIFrame* aFrame1, nsIFrame*
> aFrame2);
> 
> and using them for the assertions in the variuos "set" methods. Sounds good?

If they're only used from DEBUG code, they should be ifdef DEBUG, but otherwise yes.


(In reply to comment #53)
> (In reply to comment #48)
> > +    if ((NS_FRAME_IS_NOT_COMPLETE(aStatus) ||
> > (pfd->mFrame->GetNextContinuation() && !pfd->mFrame->GetNextInFlow())) 
> > +        && !pfd->GetFlag(PFD_ISLETTERFRAME)) {
> >        // Only apply end margin for the last-in-flow. Zero this out so
> > 
> > I think you could simplify the first line to just
> > +    if (pfd->mFrame->GetNextContinuation()
> > ... right?
> 
> David, can you please help me answer this? This is basically the inverse of the
> condition I changed in nsInlineFrame::ReflowFrames() (for paddings and
> borders), which you suggested to me on IRC. I assume that if one of them can be
> simplified, then so can the other.

I suppose it depends where the code is.  I don't recall when we create continuations given that a frame is not complete -- if that's already happened, then NS_FRAME_IS_NOT_COMPLETE should be equivalent to GetNextInFlow, and the code could be simplified as you describe.  If not, the !GetNextInFlow should probably still go anyway because it's based on out-of-date information and the other tests cover what you want anyway.

Comment 56

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

> > > +    if ((NS_FRAME_IS_NOT_COMPLETE(aStatus) ||
> > > (pfd->mFrame->GetNextContinuation() && !pfd->mFrame->GetNextInFlow())) 
> > > +        && !pfd->GetFlag(PFD_ISLETTERFRAME)) {
> > >        // Only apply end margin for the last-in-flow. Zero this out so
> > > 
> > > I think you could simplify the first line to just
> > > +    if (pfd->mFrame->GetNextContinuation()
> > > ... right?
> > 
> > David, can you please help me answer this? This is basically the inverse of
> > the condition I changed in nsInlineFrame::ReflowFrames() (for paddings and
> > borders), which you suggested to me on IRC. I assume that if one of them
> > can be simplified, then so can the other.
> 
> I suppose it depends where the code is.  I don't recall when we create
> continuations given that a frame is not complete -- if that's already
> happened, then NS_FRAME_IS_NOT_COMPLETE should be equivalent to
> GetNextInFlow, and the code could be simplified as you describe.  If not, the
> !GetNextInFlow should probably still go anyway because it's based on
> out-of-date information and the other tests cover what you want anyway.

Actually the next-in-flow hasn't been created yet there, so my suggestion is no good.

Comment 57

[Uri Bernstein]

Attachment: patch v4

Changes from previous version:

- Added same-type assertions in SetPrev/Next* methods per comment #43.
- Added no-loop assertions in the same methods per comment #46
- Brand new IID for nsIFrame (I need to do this, right?)
- Use nsCSSFrameConstructor::CreateContinuingFrame() both in SplitInlineAncestors(), and in CreateBidiContinuations. Added an optional aIsFluid argument to CreateContinuingFrame so that this will be possible.
- Replaced IsDecsendable() [sic] with IsBidiLeaf(), with reveresed logic.
- Less ignoring of return values in nsBidiPresUtis.
- Changed the table code to use mNextContinuation per comment #45, and then back to GetNextInFlow() per comment #48 :-)
- Didn't forget to include nsBidiPresUtils.h in the patch.
- Updated to head.

Regarding comment #48:
> +  // Insert aFrameList after the last continuation of aPrevFrame.
>    if (aPrevFrame) {
> +    aPrevFrame = aPrevFrame->GetLastContinuation();
>    }
>    return aParentFrame->InsertFrames(aListName, aPrevFrame, aFrameList);
>
> Seems like things could go horribly wrong if
> aPrevFrame->GetLastContinuation()'s parent is not aParentFrame. Should we reget
> the parent frame? Why do we even take aParentFrame as a parameter here???

I think we need aParentFrame because aPrevFrame might be null.
With smontagu's help, I verified that InsertFrames should actually never be called with an aPrevFrame which has forward continuations. This used to be true for in-flows, and with the current patch, it should be true for non-fluid continuations as well. So I removed the problematic code above, and replaced it with an assertion that aPrevFrame is either null or has no next continuation.

Regarding manipulating frame lists directly in SplitInlineAncestors(): I ran into some problems when trying to do this, so I'll take up on dbaron's suggestion to push this to a follow-up patch.

Comment 58

[Uri Bernstein]

(In reply to comment #48)
> +  // If a Bidi frame has a continuation frame, remove it before the first
> frame
> +  nsIFrame* nextContinuation =
> aOldFrame->GetLastInFlow()->GetNextContinuation();
> +  if (nextContinuation) {
> +    RemoveFrame(aParentFrame, aListName, nextContinuation);
> 
> Ditto...

Errr... I somehow managed to miss that. I hope what I said about InsertFrames() holds also ForRemoveFrame(), but I'll have to check, and that might only happen a couple of days from now.

Comment 59

[Mohsen Naderi]

Hi Uri,
  I was trying to put our patch for 121633 into previous version of this one since it makes all the inline frame for bidi in one place. I got your new patch and I want to do it again on this one. Do you think it worths trying?

Comment 60

[Uri Bernstein]

(In reply to comment #59)
> Hi Uri,
>   I was trying to put our patch for 121633 into previous version of this one
> since it makes all the inline frame for bidi in one place. I got your new patch
> and I want to do it again on this one. Do you think it worths trying?
> 

I expect patch v4 to be pretty close to the version that will land, at least as far as nsBidiPresUtils is concerned, so I would say it's worth trying.
Then again, you can never be sure, so if you want to avoid the possibility of having to do your work again (and again), you might want to wait for this to actually land.

Comment 61

[Uri Bernstein]

Attachment: patch v4.1

Two changes from v4:

- Implemented GetFirst/LastContinuation() in ns[Continuing]TextFrame, which I somehow forgot to do before. IMO, It sucks that nsTextFrame does not derive from nsSplittableFrame. what's the reason for that?

- For RemoveFrame (comment #58): The logical thing to do is for RemoveFrame to remove all continuations (not just NIFs as it currently does). For that, I changed GetNextInFlow() to GetNextContinuation() in nsInlineFrame::RemoveFrame(), and I wanted to do the same for nsBlockFrame::DoRemoveFrame(). However, DoRemoveFrame() is also used during reflow, where it should specifically only remove fluid continuations. So I had to add a boolean parameter to indicate whether we want to remove all continuations or just the NIFs.

Sorry again for the many patch versions. I really appreciate all the comments.

Comment 62

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

(In reply to comment #61)
> - Implemented GetFirst/LastContinuation() in ns[Continuing]TextFrame, which I
> somehow forgot to do before. IMO, It sucks that nsTextFrame does not derive
> from nsSplittableFrame. what's the reason for that?

Saves one pointer in in nsTextFrame (the prev-in-flow).

Comment 63

[Uri Bernstein]

I created an updated version of
http://wiki.mozilla.org/Gecko:Continuation_Model here: 
http://wiki.mozilla.org/User:Uri/Gecko:Continuation_Model (you can view the changes here: http://wiki.mozilla.org/index.php?title=User:Uri/Gecko:Continuation_Model&diff=0&oldid=17497).

You might want to review this for accuracy, style, etc. I hope to replace the current article with the updated version once the patch lands.

Comment 64

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

Comment on attachment 212332 [details] [diff] [review]
patch v4.1

r+sr=dbaron still stands, but a few comments:

the |const|-ness on the nsIFrame methods is a bit fishy, since you can walk around the tree and back to cast away const, but the existing const frame accessors on nsIFrame have the same problem.

nsContinuingTextFrame::GetFirstContinuation has a problems:
 * you call GetFirstContinuation recursively where you meant to call GetPrevInFlow, such that you'll just go into an infinite loop

GetLastContinuation has a different problem:  you call the virtual function GetNextContinuation twice, when you could use a variable and just use it once, e.g.,

while ((nsIFrame *next = lastInFlow->GetNextContinuation))
  lastInFlow = next;

But actually, that fix belongs in GetFirstContinuation.  In GetLastContinuation you *can* just use mNextContinuation directly.

Comment 65

[Uri Bernstein]

Attachment: patch v4.2

Fixed the issues in GetLastContinuation and GetFirstContinuation.

I'll land this patch tomorrow moning/noon PDT, if no-one has objections.

Comment 66

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

Comment on attachment 212498 [details] [diff] [review]
patch v4.2

> nsIFrame* nsSplittableFrame::GetFirstInFlow() const
> nsIFrame* nsSplittableFrame::GetLastInFlow() const

These two could also benefit from only calling the virtual function once.

>+nsIFrame*
>+nsContinuingTextFrame::GetFirstContinuation() const
>+{
>+  // Can't cast to |nsContinuingTextFrame*| because the first one isn't.
>+  nsIFrame *firstContinuation,
>+  *previous = NS_CONST_CAST(nsIFrame*,
>+                            NS_STATIC_CAST(const nsIFrame*, this));

In this case only, you could actually start with previous = mPrevContinuation, since you know an nsContinuingTextFrame has one that's non-null.

Comment 67

[Uri Bernstein]

Attachment: patch v4.3

Fixed.

and s/PDT/PST/ in my previous comment (not that it matters in this case).

Comment 68

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

Comment on attachment 212504 [details] [diff] [review]
patch v4.3

You should double-parenthesize assignment within while, since it avoids compiler warnings, e.g.:

while ((nsIFrame *prev = firstInFlow->GetPrevInFlow()))

Also, things like:

firstInFlow = (nsSplittableFrame*)...;

are better written as:

firstInFlow = NS_STATIC_CAST(nsSplittableFrame*, ...);

since use of NS_STATIC_CAST means most compilers will give an error if it turns into the equivalent of a reinterpret_cast for some reason (which can happen if appropriate headers aren't included!).  There are a bunch of these in various places in nsSplittableFrame.cpp and nsTextFrame.cpp.

You don't need to attach a new patch for review for each set of these comments, unless you really want to.

Comment 69

[Uri Bernstein]

(In reply to comment #68)
> There are a bunch of these in various
> places in nsSplittableFrame.cpp and nsTextFrame.cpp.

I changed these places to use NS_STATIC_CAST, and also changed some "un-consting" casts to use NS_CONST_CAST.

> You don't need to attach a new patch for review for each set of these comments,
> unless you really want to.

OK, thanks. I don't really want to.

Comment 70

[Mohsen Naderi]

Dear Uri,
  I tested the patch and it seems that it does not process margins and paddings well. Actually it does not work even for ltr inline frames. Please try running it on test case for bug 121633. Unpatched code shown ltr properly but it has problem with rtl, but patched code does not show any spacing.

Comment 71

[Haamed Gheibi]

This patch does not consider margins.
About paddings and borders the problem is:
  When it splits an inline frame to bidi continuation frames, left-padding and left-border goes with primary frame (first frame in logical order) and right-padding and right-border goes with last continuation frame. (again in logical order)
But as you know the correct layout is to apply left-border and left-padding to first continuation frame in visual order (left most you see) and right ones the same. This first and last will be determined after reordering.
So it seems we must tune frames after reordering.

Comment 72

[Uri Bernstein]

(In reply to comment #70 and comment #71)

Regarding paddings and borders: The main focus of this bug was not to fix paddings and borders. However, since the old implementation of Reorder() etc. no longer made sense with this patch, I saw the opportunity to re-write them in a way which is both much simpler, and also significantly improves handeling of padding and borders (e.g, it fixes all the testcases in bug 237370 an bug 258928 and their dependencies). As I wrote several times before, I am fully aware that this is not a complete solution, but I think it's much better than what we had.

Regarding margins: This patch was not supposed to fix margins at all. However, you are correct that as it stands it causes a regression: the disappearing-margins problem which previously affected only RTL lines or LTR lines with complex bidi ordering, now affects everything (if there is RTL content anywhere on the page). The previous implementation avoided this because it short-circuited RepositionInlineFrames() for simple LTR lines. In the new implementation, I had to remove that short-circuit. However, now that you pointed out to me the regression, I'll shortly attach a version of the patch which partially restores the short-circuit mechanism, so that the status quo regarding margins will be preserved (or perhaps even slightly improved upon in some cases). 

Comment 73

[Uri Bernstein]

Attachment: patch v4.4

This patch contains the margin-regression fix I mentioned in the previous comment, as well as a slight clean-up of the logic in EnsureBidiContinuation(), and the fixes to casts discussed above.

Comment 74

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

Comment on attachment 212600 [details] [diff] [review]
patch v4.4

> -  PRBool inWord = lineLayout.InWord() || ((nsnull != prevInFlow) && (((nsTextFrame*)prevInFlow)->mState & TEXT_FIRST_LETTER));
> +  PRBool inWord = lineLayout.InWord() || ((nsnull != prevInFlow) && ((NS_STATIC_CAST(nsTextFrame*, prevInFlow))->mState & TEXT_FIRST_LETTER));

You've got an extra set of parentheses around the NS_STATIC_CAST here.

This repositioning code does need to be fixed, perhaps even redesigned.  Could you make sure there's a bug on that?  (And when doing it, do consider the specs, but don't always assume that they're correct -- the CSS spec is written mostly by people who know nothing about Bidi.  But please do raise issues that you find.)

Comment 75

[Uri Bernstein]

(In reply to comment #74)
> (From update of attachment 212600 [details] [diff] [review] [edit])
> > -  PRBool inWord = lineLayout.InWord() || ((nsnull != prevInFlow) && (((nsTextFrame*)prevInFlow)->mState & TEXT_FIRST_LETTER));
> > +  PRBool inWord = lineLayout.InWord() || ((nsnull != prevInFlow) && ((NS_STATIC_CAST(nsTextFrame*, prevInFlow))->mState & TEXT_FIRST_LETTER));
> 
> You've got an extra set of parentheses around the NS_STATIC_CAST here.
> 

Will fix this for checkin.

> This repositioning code does need to be fixed, perhaps even redesigned.  Could
> you make sure there's a bug on that?  (And when doing it, do consider the
> specs, but don't always assume that they're correct -- the CSS spec is written
> mostly by people who know nothing about Bidi.  But please do raise issues that
> you find.)
> 

Yes, I will certainly post a followup bug on this.
Actually, I think the CSS standard does make sense here (IIRC, smontagu said that Hixie consulted him when working on that section of the standard). It does seem a bit difficult to implement, but Mohsen and Haamed might have a solution.

Comment 76

[Uri Bernstein]

Checked in. I'll deal with filing follow-ups, updating dependencies, etc. within the next day.

Checking in layout/generic/nsSplittableFrame.h;
/cvsroot/mozilla/layout/generic/nsSplittableFrame.h,v  <--  nsSplittableFrame.h
new revision: 3.25; previous revision: 3.24
done
Checking in layout/generic/nsSplittableFrame.cpp;
/cvsroot/mozilla/layout/generic/nsSplittableFrame.cpp,v  <--  nsSplittableFrame.cpp
new revision: 3.32; previous revision: 3.31
done
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.547; previous revision: 1.546
done
Checking in layout/generic/nsIFrame.h;
/cvsroot/mozilla/layout/generic/nsIFrame.h,v  <--  nsIFrame.h
new revision: 3.304; previous revision: 3.303
done
Checking in layout/generic/nsFrame.h;
/cvsroot/mozilla/layout/generic/nsFrame.h,v  <--  nsFrame.h
new revision: 3.238; previous revision: 3.237
done
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.617; previous revision: 3.616
done
Checking in layout/generic/nsBlockFrame.h;
/cvsroot/mozilla/layout/generic/nsBlockFrame.h,v  <--  nsBlockFrame.h
new revision: 3.213; previous revision: 3.212
done
Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.753; previous revision: 3.752
done
Checking in layout/generic/nsColumnSetFrame.cpp;
/cvsroot/mozilla/layout/generic/nsColumnSetFrame.cpp,v  <--  nsColumnSetFrame.cpp
new revision: 3.18; previous revision: 3.17
done
Checking in layout/generic/nsContainerFrame.cpp;
/cvsroot/mozilla/layout/generic/nsContainerFrame.cpp,v  <--  nsContainerFrame.cpp
new revision: 1.250; previous revision: 1.249
done
Checking in layout/generic/nsFirstLetterFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFirstLetterFrame.cpp,v  <--  nsFirstLetterFrame.cpp
new revision: 1.54; previous revision: 1.53
done
Checking in layout/generic/nsHTMLCanvasFrame.cpp;
/cvsroot/mozilla/layout/generic/nsHTMLCanvasFrame.cpp,v  <--  nsHTMLCanvasFrame.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in layout/generic/nsImageFrame.cpp;
/cvsroot/mozilla/layout/generic/nsImageFrame.cpp,v  <--  nsImageFrame.cpp
new revision: 1.373; previous revision: 1.372
done
Checking in layout/generic/nsInlineFrame.cpp;
/cvsroot/mozilla/layout/generic/nsInlineFrame.cpp,v  <--  nsInlineFrame.cpp
new revision: 3.247; previous revision: 3.246
done
Checking in layout/generic/nsPageFrame.cpp;
/cvsroot/mozilla/layout/generic/nsPageFrame.cpp,v  <--  nsPageFrame.cpp
new revision: 3.151; previous revision: 3.150
done
Checking in layout/generic/nsLineLayout.cpp;
/cvsroot/mozilla/layout/generic/nsLineLayout.cpp,v  <--  nsLineLayout.cpp
new revision: 3.230; previous revision: 3.229
done
Checking in layout/base/nsBidiPresUtils.h;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.h,v  <--  nsBidiPresUtils.h
new revision: 1.17; previous revision: 1.16
done
Checking in layout/base/nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v  <--  nsBidiPresUtils.cpp
new revision: 1.64; previous revision: 1.63
done
Checking in layout/base/nsFrameManager.cpp;
/cvsroot/mozilla/layout/base/nsFrameManager.cpp,v  <--  nsFrameManager.cpp
new revision: 1.222; previous revision: 1.221
done
Checking in layout/base/nsCSSFrameConstructor.h;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.h,v  <--  nsCSSFrameConstructor.h
new revision: 1.204; previous revision: 1.203
done
Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1196; previous revision: 1.1195
done
Checking in layout/tables/nsTableCellFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableCellFrame.cpp,v  <--  nsTableCellFrame.cpp
new revision: 3.374; previous revision: 3.373

done
Checking in layout/tables/nsTableColGroupFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableColGroupFrame.cpp,v  <--  nsTableColGroupFrame.cpp
new revision: 3.151; previous revision: 3.150
done
Checking in layout/tables/nsTableFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableFrame.cpp,v  <--  nsTableFrame.cpp
new revision: 3.630; previous revision: 3.629
done
Checking in layout/tables/nsTableRowFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableRowFrame.cpp,v  <--  nsTableRowFrame.cpp
new revision: 3.377; previous revision: 3.376
done
Checking in layout/tables/nsTableRowGroupFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableRowGroupFrame.cpp,v  <--  nsTableRowGroupFrame.cpp
new revision: 3.353; previous revision: 3.352
done

Comment 77

[Boris Zbarsky [:bzbarsky]]

Uri, this is great stuff!  Thanks for doing it!  You'll update <http://wiki.mozilla.org/Gecko:Continuation_Model>, right?

Also, is it me or could we basically remove the AppendFrames/InsertFrames/RemoveFrame methods on nsFrameManager now?  If we can, do we want to?

Comment 78

[Uri Bernstein]

(In reply to comment #77)
> Uri, this is great stuff!  Thanks for doing it!  

My pleasure :-). And many many thanks to all the people who commented here and helped me write the patch, especially dbaron.

>You'll update <http://wiki.mozilla.org/Gecko:Continuation_Model>, right?

Already did that (please review my changes and correct anything needing correction).

> Also, is it me or could we basically remove the
> AppendFrames/InsertFrames/RemoveFrame methods on nsFrameManager now?  If we
> can, do we want to?

They existed before the IBMBIDI stuff (which I removed) was added to them:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/base&command=DIFF_FRAMESET&file=nsFrameManager.cpp&rev2=1.70&rev1=1.69
so perhaps there's some justification for their existance.

Comment 79

[Boris Zbarsky [:bzbarsky]]

> They existed before the IBMBIDI stuff (which I removed) was added to them:

Yeah, I know.  I still suspect they're bogus.  Should get a bug filed on maybe removing them...

Also, I just ran into bug 328264, which prompted me to look at all the GetNextInFlow() callers outside layout/.  I suspect that every single one of these should be using GetNextContinuation(), but I wouldn't stake my life on it.  Should get bugs filed on the relevant modules (accessibility, both typeahead finds, spatial navigation, and DOM inspector), I guess.  The DOM inspector code should definitely be using GetNextContinuation()...

Comment 80

[Uri Bernstein]

(In reply to comment #79)

> Also, I just ran into bug 328264, which prompted me to look at all the
> GetNextInFlow() callers outside layout/.  I suspect that every single one of
> these should be using GetNextContinuation(), but I wouldn't stake my life on
> it.  Should get bugs filed on the relevant modules (accessibility, both
> typeahead finds, spatial navigation, and DOM inspector), I guess.  The DOM
> inspector code should definitely be using GetNextContinuation()... 

I actually saw the DOM inspector problem earlier and meant to file a bug on it, but then forgot. I'll do this later today. I'll also try to look at the other places you mention and see if I can file bugs on them.

Comment 81

[Simon Montagu :smontagu]

*** Bug 358768 has been marked as a duplicate of this bug. ***

Comment 82

[Simon Montagu :smontagu]

I think this is an important bug to have regression tests for. Uri, I can throw together reftests from the testcases in here and various dependencies, but if you have some free time for this and you know of some other edge cases that would be good to test, please feel free to add them too.

Comment 83

[Simon Montagu :smontagu]

Reftest based on attachment 187551 [details] checked in as part of http://hg.mozilla.org/mozilla-central/rev/b7b8d7526192