421239 - {inc}Inconsistent margin after emptying paragraph in standards mode

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P2)

Description

[Jesse Ruderman]

The testcase and reference should look the same, since they have the same final DOM.  But in the testcase, there's more space above the second (nonempty) paragraph.

This reminds me of bug 372632, bug 373298, bug 405517, and bug 418766.

Comment 1

[Jesse Ruderman]

Attachment: testcase (dynamic)

Comment 2

[Jesse Ruderman]

Attachment: reference (static)

Comment 3

[Daniel Holbert [:dholbert]]

I see this using
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4) Gecko/2008030318 Firefox/3.0b4

OS/Hardware --> All

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

This testcase has the margins explicitly defined, and only uses margins on the first block (not both)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: reference 2 (broken, ignoreme)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: reference 2

(oops, forgot to take out the 'A' in the first version of reference 2)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

testcase 3 has no top margin on the first div -- just a 50px bottom margin.

However, after the div is emptied, it looks like we add a top margin, or something -- the outcome looks just like that of testcase 2.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: reference 3

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to bug 418766 comment #10)
> In other words, I'm assuming that in all situations where line->CachedIsEmpty()
> returns true, line->mBounds.width should be 0.

I think this assumption stated in bug 418766 doesn't hold up in the testcases here, and so bug 418766's checking caused this bug.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: patch v1

This patch loosens the assumption quoted in Comment 9 to this:

I'm assuming that in all situations where line->CachedIsEmpty()
returns true, line->mBounds.width OR height should be 0.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: patch v1a (with reftests)

same as patch v1, plus reftests based on testcases 2 and 3.

Comment 12

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

Which line in these cases is empty and has height 0 but width nonzero?

Comment 13

[Daniel Holbert [:dholbert]]

The first one (the one from which the "A" is removed)

I *think* it had the width of the viewport, but I'll double-check.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: framedumps from testcase 3

Here's a framedump pre-JS-tweak and during-JS-tweak of testcase 3.  (using a minimized version without whitespace to simplify the frame tree, available at http://people.mozilla.com/~dholbert/tests/421239/test3.html)

Quoting from a comment in this attached framedump:

"Notice that the very first line has a width of 48240 (the same
width as the body block) and a height of 0.  That line "is empty" by height
standards, but not by width standards."

Comment 15

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

This won't work if you put an empty span in the <div> so that it has height as well as width, i.e. combine this test with the test in bug 418766. Right?

Maybe we should just scrub this optimization and hope that it doesn't matter.

Comment 16

[Daniel Holbert [:dholbert]]

This patch still works if I just add an empty span to div "a" (after the character).

IIRC from bug 418766, an empty span ONLY gets a height if we're in quirks mode.
And it seems in this bug that an empty div only gets a width in standards mode.

So, to phrase that a different way, I'm hoping this assumption holds:
  - in quirks mode, it's impossible to have an empty line with 0 width
  - in standards mode, it's impossible to have an empty line with 0 height

And hence, you'd never be able to have an empty line with nonzero height *and* width.  In which case the optimization in patch v1a would work.

Gonna play with testcases a bit more to verify that this is all true.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 307779 [details] [diff] [review]
patch v1a (with reftests)

canceling review request for now; may re-request later when I have more info about how foolproof the optimization is.

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to comment #16)
> 
> So, to phrase that a different way, I'm hoping this assumption holds:
>   - in quirks mode, it's impossible to have an empty line with 0 width
>   - in standards mode, it's impossible to have an empty line with 0 height

Sorry, I got that backwards -- s/0/nonzero.

i.e., I'm hoping this assumption holds:
   - in quirks mode, it's impossible to have an empty line with nonzero width
   - in stds mode, it's impossible to have an empty line with nonzero height

Comment 19

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

That sounds fragile.

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: patch v2

(In reply to comment #19)
> That sounds fragile.

Yeah, I kinda agree... even if the assumption in comment 18 is correct, it seems like that could change in the future.

OK, this patch just nixes the optimization, as suggested in comment 15.

Comment 21

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

Comment on attachment 308493 [details] [diff] [review]
patch v2

ok, watch very carefully for perf changes

Comment 22

[Daniel Holbert [:dholbert]]

I'm planning on landing this tonight, if/when the tree is quiet, so I can watch for perf changes.

Comment 23

[Daniel Holbert [:dholbert]]

patch v2 landed:

Checking in generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.940; previous revision: 3.939
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/421239-1-ref.html,v
done
Checking in reftests/bugs/421239-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/421239-1-ref.html,v  <--  421239-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/421239-1.html,v
done
Checking in reftests/bugs/421239-1.html;
/cvsroot/mozilla/layout/reftests/bugs/421239-1.html,v  <--  421239-1.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/421239-2-ref.html,v
done
Checking in reftests/bugs/421239-2-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/421239-2-ref.html,v  <--  421239-2-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/421239-2.html,v
done
Checking in reftests/bugs/421239-2.html;
/cvsroot/mozilla/layout/reftests/bugs/421239-2.html,v  <--  421239-2.html
initial revision: 1.1
done

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to comment #21)
> ok, watch very carefully for perf changes
(In reply to comment #22)
> I'm planning on landing this tonight, if/when the tree is quiet, so I can watch
> for perf changes.

Perf impact seems negligible -- the mac & linux tp, tp2, tdhtml, txul, and ts numbers all stayed within their range of noise for the day for the few cycles since this landed. (Most of the numbers went down, actually, though I'm pretty sure that's from noise & not due to a perf win. :))

I don't have Windows numbers yet, due to the 2+ hour cycle times on that machine. :(

Comment 25

[Jesse Ruderman]

Marking in-testsuite+ because tests were included in the patch.