1145218 - block frames with orthogonal writing mode need their own float manager

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - add floatmanager writing-mode assertions

As proposed in bug 1142318 comment 23 (based on :dbaron's patch mentioned in bug 1114329), I tried adding the attached patch to assert if we're using a float manager whose block direction does not match what the caller expects.

Turns out these assertions fire frequently as soon as we start using vertical writing modes; e.g. a simple testcase such as

  data:text/html,<div style="writing-mode:vertical-lr;inline-size:150px">
    <p><img style="float:left" src="foo">The quick brown fox jumps over the lazy dog.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Require a new float manager if a block frame has a different writing-mode to its parent's

Is this what we want to do? I'm assuming we should not use the same float manager for vertical-lr and vertical-rl, as the block dir will be opposite.

Comment 2

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

Comment on attachment 8580131 [details] [diff] [review]
Require a new float manager if a block frame has a different writing-mode to its parent's

I think it would be better to set the NS_BLOCK_FLOAT_MGR bit; we check that bit in other places too.

Comment 3

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

(And it probably also requires NS_BLOCK_MARGIN_ROOT, which is separate, since I think we shouldn't be attempting to collapse margins per http://dev.w3.org/csswg/css2/box.html#collapsing-margins since blocks that establish orthogonal flows establish new block formatting contexts.  At least, the writing-modes spec better say that they do...)

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Require a new float manager if a block frame has a different writing-mode to its parent's

This sets the suggested bits in nsBlockFrame::Init(). Is there any situation where the frame might be reparented such that we'd need to reset/clear these bits subsequently, or can we assume that if things change that much, we'll be reconstructing frames anyway?

Comment 5

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

Comment on attachment 8580753 [details] [diff] [review]
Require a new float manager if a block frame has a different writing-mode to its parent's

r=dbaron, though maybe it's worth a comment citing the spec http://dev.w3.org/csswg/css-writing-modes/#block-flow which says that:

If a box has a different block flow direction than its containing block:
...
    If the box is a block container, then it establishes a new block formatting context.

Comment 6

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

Oh, and one nit:

drop the "StyleContext()->" in both places; there's a StyleVisibility() on nsIFrame that
forwards to the style context, and is preferred.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Make the float manager's writing-mode field debug-only, and assert that it matches what callers are passing in

Updated patch 1 following the recent float manager rework.

Comment 8

[Simon Montagu :smontagu]

Comment on attachment 8581545 [details] [diff] [review]
Make the float manager's writing-mode field debug-only, and assert that it matches what callers are passing in

Review of attachment 8581545 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFloatManager.cpp
@@ -404,5 @@
>    // reflow mentioned above) and then from B to C during the subsequent
>    // reflow. In the typical case A and C will be the same, but not always.
>    // Allowing mFloatDamage to accumulate the damage incurred during both
>    // reflows ensures that nothing gets missed.
> -  aState->mWritingMode = mWritingMode;

Don't we want to make this #ifdef DEBUG instead of removing it completely?

@@ -419,5 @@
>  nsFloatManager::PopState(SavedState* aState)
>  {
>    NS_PRECONDITION(aState, "No state to restore?");
>  
> -  mWritingMode = aState->mWritingMode;

Ditto

::: layout/generic/nsFloatManager.h
@@ -88,5 @@
>    struct SavedState {
>      explicit SavedState() {}
>    private:
>      uint32_t mFloatInfoCount;
> -    mozilla::WritingMode mWritingMode;

Ditto

@@ -288,5 @@
>  
>    void AssertStateMatches(SavedState *aState) const
>    {
> -    NS_ASSERTION(aState->mWritingMode == mWritingMode &&
> -                 aState->mLineLeft == mLineLeft &&

Ditto

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #8)

> ::: layout/generic/nsFloatManager.cpp
> @@ -404,5 @@
> >    // reflow mentioned above) and then from B to C during the subsequent
> >    // reflow. In the typical case A and C will be the same, but not always.
> >    // Allowing mFloatDamage to accumulate the damage incurred during both
> >    // reflows ensures that nothing gets missed.
> > -  aState->mWritingMode = mWritingMode;
> 
> Don't we want to make this #ifdef DEBUG instead of removing it completely?

As far as I could see, including the writing mode in the SavedState seemed to be completely redundant; we don't modify the float manager's mode after initialization, so it never needs to be saved and restored either. Or did I miss something there?

Comment 10

[Simon Montagu :smontagu]

Comment on attachment 8581545 [details] [diff] [review]
Make the float manager's writing-mode field debug-only, and assert that it matches what callers are passing in

Review of attachment 8581545 [details] [diff] [review]:
-----------------------------------------------------------------

Oh well, if you put it that way...

Comment 11

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bad0f7f06864
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf6b633b054

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for making windows reftests frequently fail:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd1b08994a7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5864129e5d5f

https://treeherder.mozilla.org/logviewer.html#?job_id=8366367&repo=mozilla-inbound

Comment 13

[Jonathan Kew [:jfkthame]]

It looks like the reftest failures here are spurious, inasmuch as they're not an "error" in our layout but rather an artifact where Windows positions glyphs erratically on the pixel grid: we're getting a one-pixel offset of the baseline between adjacent glyphs within the same element and textrun, which should be rendered with the exact same x-coordinate.

The patch here has affected this, presumably, because the new float manager we're creating for the vertical writing-mode elements means that available-space computations are working within a new coordinate space, and we end up 

It's possible that we can mitigate this somehow via device-pixel rounding or snapping at some level, but it's not a new problem and it shouldn't block this bug. We already have some fuzz-annotations that are due to this same issue, so I propose to mark these tests fuzzy as well and re-land these patches.

Comment 14

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc35109e04d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/87c03b8a7f37
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a99cd9a8187

Comment 15

[Jonathan Kew [:jfkthame]]

FTR, I just filed bug 1150250 on the tests that I marked fuzzy here.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc35109e04d4
https://hg.mozilla.org/mozilla-central/rev/87c03b8a7f37
https://hg.mozilla.org/mozilla-central/rev/0a99cd9a8187

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/404a3f74b2793339294cbdfd522df47d612192a3
Bug 1145218 followup - Properly parenthesize macro argument.  No review.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/404a3f74b279