423130 - Inconsistent layout with padding, removing RLM

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

The testcase and reference have the same final DOM, but the testcase is displayed with the rectangle taller.

This bug report was brought to you by refdyn and 
layout/reftests/bugs/421419-1.html.

Comment 1

[Jesse Ruderman]

Attachment: testcase (dynamic)

Comment 2

[Jesse Ruderman]

Attachment: reference (static)

Comment 3

[Uri Bernstein]

This is actually a bit worrying. It appears that inline bidi continuations don't get deleted when their content is deleted.

Comment 4

[Uri Bernstein]

In nsBidiPresUtils::Resolve(), there's an asymmetry between the way we split ancestor frames into bidi continuations, and the way we "join" them (i.e., convert bidi continuations into fluid continuations).

Splitting ancestor frames (implemented by SplitInlineAncestors) happens whenever we reach an end of run (regardless of the type of the leaf frame we're looking at).
"Joining" ancestors, on the other hand, happens as part of RemoveBidiContinuation(), which is only called when we encounter a text node that needs to be "joined".

So the case we're failing on is when the joining should only start at the parent level (because the leaves aren't all text nodes).

What complicates things further is that RemoveBidiContinuation() does joining "in batch" for all remaining leaf frames belonging to the same content. So it's not clear that it could just be replaced by something like JoinInlineAncestors() (which would be invoked similarly to the way SplitInlineAncestors() is). My current thinking is that we might need a JoinInlineAncestors() on top of the joining we currently have.

All of this makes my head spin (which is why I'm writing it down). I'll see if I can find time to work on this, but I expect to be pretty busy this week.

Comment 5

[Uri Bernstein]

Attachment: like this

What I said in the previous comment. This might not be the most elegant way to do this, but I think it's the safest at this point.

Comment 6

[Uri Bernstein]

Oops, ignore that added 'const' on the last changed line.

Comment 7

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

Comment on attachment 309674 [details] [diff] [review]
like this

sr=dbaron assuming smontagu's review; hopefully he understands this better than I do.

One comment is that you could convert:

    if (frame && fragmentLength <= 0 && runLength <= 0) {
      // existing code
    }
    else if (frame && fragmentLength <= 0 && runLength > 0) {
      // your new code
    }

to be:

    if (frame && fragmentLength <= 0) {
      if (runLength <= 0) {
        // existing code
      } else {
        // your new code
      }
    }
... assuming you also update/move the comments accordingly.

Also, it seems like JoinInlineAncestors should either find frames to join all the way up, or not find any.  (Or can the latter case ever happen?)  Is that the case?  If so, I wonder if it should break out of the loop if (!next).

Comment 8

[Uri Bernstein]

(In reply to comment #7)

> Also, it seems like JoinInlineAncestors should either find frames to join all
> the way up, or not find any.  (Or can the latter case ever happen?)  Is that
> the case?  If so, I wonder if it should break out of the loop if (!next).
> 

It could start finding them somewhere in the middle, for a case like: <b><i>ltr</i><i>RTL</i></b>. There's nothing to join in the <i>s' level, but the <b> continuation should be joined.

Comment 9

[Simon Montagu :smontagu]

Comment on attachment 309674 [details] [diff] [review]
like this

r=me, assuming that it passes reftests, and passes crashtests with no new assertions (probably more important in this case).

Comment 10

[Damon Sicore (:damons)]

Is this checked in?  If not, ready to request approval for beta5?

Comment 11

[Uri Bernstein]

Not checked in yet, as I haven't had time to run all the tests yet as Simon requested.

Comment 12

[Uri Bernstein]

Attachment: patch for checkin

Includes the change suggested by David, and a reftest.

Comment 13

[Uri Bernstein]

Attachment: The real reftest

Comment 14

[Damon Sicore (:damons)]

Comment on attachment 310860 [details] [diff] [review]
patch for checkin

a1.9b5+=damons

Comment 15

[Damon Sicore (:damons)]

Beltzner, status on this is Uri is running tests to make sure there are no assertions.  It's 1:30am his time, so he might wait until morning his time to check in due to the fact it's a long running process.

Comment 16

[Uri Bernstein]

Tests found no regressions, and I'm ready to submit, but tree is orange now. So I'll go have some sleep and try submitting in the morning. If someone wants to submit attachment 310860 [details] [diff] [review] for me that's fine, but you might want to leave out the reftest (the other patch), as the reftest.list part of it is corrupt.

Comment 17

[Uri Bernstein]

Checked in:

Checking in mozilla/layout/base/nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v  <--  nsBidiPresUtils.cpp
new revision: 1.113; previous revision: 1.112
done
Checking in mozilla/layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.406; previous revision: 1.405
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/423130-1.html,v
done
Checking in mozilla/layout/reftests/bugs/423130-1.html;
/cvsroot/mozilla/layout/reftests/bugs/423130-1.html,v  <--  423130-1.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/423130-1-ref.html,v
done
Checking in mozilla/layout/reftests/bugs/423130-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/423130-1-ref.html,v  <--  423130-1-ref.html
initial revision: 1.1
done

Comment 18

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050621 Firefox/3.0pre and the testcases from this bug

--> Verified fixed