Closed Bug 1183431 Opened 5 years ago Closed 5 years ago

"ASSERTION: block direction mismatch" and incorrect static position for positioned element with writing-mode and position: fixed or absolute

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jruderman, Assigned: jfkthame)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(5 files, 3 obsolete files)

Attached file testcase
###!!! ASSERTION: block direction mismatch: 'hypotheticalBox.mWritingMode.GetBlockDir() == cbwm.GetBlockDir()', file layout/generic/nsHTMLReflowState.cpp, line 1530

This assertion was added in:

changeset:   https://hg.mozilla.org/mozilla-central/rev/e641f522e5ae
user:        Jonathan Kew
date:        Fri Jun 05 08:47:09 2015 +0100
summary:     Bug 1079151 - patch 1 - Update constraint calculations in nsHTMLReflowState to work with logical coordinates. r=smontagu
Attached file stack
This should fix it. Includes the testcase as a crashtest.
Attachment #8633401 - Flags: review?(dholbert)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8633401 [details] [diff] [review]
Convert writing mode if necessary when getting bStart from hypothetical-box

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

::: layout/generic/nsHTMLReflowState.cpp
@@ +1525,5 @@
>    }
>  
>    if (bStartIsAuto && bEndIsAuto) {
>      // Treat 'top' like 'static-position'
> +    if (hypotheticalBox.mWritingMode.GetBlockDir() == cbwm.GetBlockDir()) {

Why can't we assume that hypotheticalBox matches cbwm?

Note that CalculateHypotheticalBox (which sets up this variable) has this documentation:
  The values returned are [...] in the actual containing block's writing mode.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=e70e937ae75b#1181

Is that documentation wrong? Or how do we end up with a mismatched writing-mode here? (Is "the actual containing block" different from "cbwm" in this function?")
Slightly broader quote from the CalculateHypotheticalBox documentation:
  // The values returned are [...] in the actual containing block's writing mode.
  // cbrs->frame is the actual containing block

And cbrs->frame should have cbwm as its writing-mode, I think... So I don't see why we're violating the assertion here.
Flags: needinfo?(jfkthame)
The testcase here has:

  <html>
    <body>
      <div style="writing-mode: vertical-lr;">
        <div style="position: fixed;"></div>
      </div>
    </body>
  </html>

We hit the assertion when we've calculated the hypotheticalBox for the position:fixed div. That div's "actual containing block" is the outer div, which has writing-mode:vertical-lr. But the containing block whose writing mode ends up as cbwm is "this frame's CSS containing block", as found by nsIFrame::GetContainingBlock(), which for an absolutely positioned frame (such as position:fixed) will be the viewport, if there's not another positioned ancestor to use.

So cbwm is horizontal-tb / ltr, from the viewport within which the frame is being positioned; but the hypotheticalBox uses vertical-lr / ltr, from the "actual" containing block.

(The "actual containing block" term here could probably use some clarification; that's what was already in the code before I got there, IIRC, but it wasn't immediately obvious to me either.)
Flags: needinfo?(jfkthame)
So I think the comment I quoted in Comment 6 is just wrong... looks like we establish hypotheticalBox's writing-mode here:
> 1204   WritingMode wm = containingBlock->GetWritingMode();
> 1205   aHypotheticalBox.mWritingMode = wm;

Here, containingBlock is most definitely not the "actual containing block" (not as the comment defines it, as cbrs->frame).
(What I'm wondering here is, is the right fix here to *make* that comment valid?  i.e. would things be simpler if CalculateHypotheticalBox converted hypotheticalBox into cbwm before it returns?

The documentation in comment 6 and the assertion that's being violated here both seem to assume that this is the way things should be, at least.)
(In reply to Jonathan Kew (:jfkthame) from comment #7)
> So cbwm is horizontal-tb / ltr, from the viewport within which the frame is
> being positioned; but the hypotheticalBox uses vertical-lr / ltr, from the
> "actual" containing block.

(The vertical-lr wrapper-div isn't the "actual containing block", as defined by the comment I'm quoting.  It's the |containingBlock| variable, which is returned from GetHypotheticalBoxContainer(). In contrast, the "actual containing block" is cbrs->frame, per comment 6. And cbrs->frame is the ViewportFrame.)
Agreed, this does look confused. (Or should I say, I look confused.) I'll poke at it a bit tomorrow and see if I can untangle things....
For history, it looks like the "actual containing block" language was first added here, by dbaron:
  http://hg.mozilla.org/mozilla-central/rev/483aaa57f290#l11.12
and it was to distinguish between the "placeholder's containing block" (the wrapper-div in this case) vs. the "actual containing block" (the viewport in this case).

More recently, you added the documentation-line that says the returned value here is in the "actual containing block's writing mode" (which would mean the viewport's WM in this case):
  http://hg.mozilla.org/mozilla-central/diff/e641f522e5ae/layout/generic/nsHTMLReflowState.cpp#l1.80
...though we *actually* use the "placeholder's containing block" (called containingBlock) to establish the outparam's writing-mode in that commit:
  http://hg.mozilla.org/mozilla-central/diff/e641f522e5ae/layout/generic/nsHTMLReflowState.cpp#l1.111

So that's where this inconsistency between documentation & code seems to have appeared.
So the only question here is whether we should fix the code, or fix the documentation/assertion.  Anyway - thanks in advance for taking a further look!
This is essentially the same patch, but with comments updated to better reflect reality. I wondered about converting things to compute the hypotheticalBox entirely using the actual container's writing mode, but that seemed to be leading me into greater confusion.... the box-properties reftests are great for letting us know when we break this stuff. :) So while I still wonder if we might want to revisit this sometime, I think the safest option for now is to keep the existing, working code and just address the case where we're tripping that assertion. (Which was added because at the time, I wasn't sure if a mismatch could occur there, but knew that if it did, we'd need to add some writing-mode conversion. And here we are.)
Attachment #8635310 - Flags: review?(dholbert)
Attachment #8633401 - Attachment is obsolete: true
Comment on attachment 8635310 [details] [diff] [review]
Convert writing mode if necessary when getting bStart from hypothetical-box

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

From local testing, I'm not actually sure this works correctly.  (My testcase coming up, which seems to be broken.)

Could you include a reftest (not just a crashtest) which tests the new math that you're adding & shows this working correctly?  (And adjust the patch to fix the mispositioning in my upcoming testcase, as-needed.)

::: layout/generic/nsHTMLReflowState.cpp
@@ +1179,5 @@
>  
>  // Calculate the hypothetical box that the element would have if it were in
>  // the flow. The values returned are relative to the padding edge of the
> +// absolute containing block, in the writing mode of aPlaceholderFrame's
> +// nearest containing block.

I'm not sure what the word "nearest" adds/means in this context -- it seems redundant, unless I'm misunderstanding its meaning.

So: consider dropping "nearest" here, to just leave "aPlaceholderFrame's containing block".

@@ +1184,1 @@
>  // cbrs->frame is the actual containing block

s/actual/absolute/ in this comment, I think?

(Originally this comment used "actual containing block" to mean "absolute containing block", but you've removed the last such usage. So better to be consistent & call this the "absolute containing block"; or alternately, just delete this sentence, since it's now less-related to the documentation before it.)
With the current patch applied, "bbb" is off to the right here for some reason. (And if I remove everything before the pink div, it's off to the left instead.)

It should be inside the pink div.
(I think the problem with my attached testcase is that we're setting offsets.IStart and offsets.BStart to the same value.  I think we need some special-case code -- like the code you're adding here -- around the offsets.IStart assignment, a little higher up in InitAbsoluteConstraints.

Ideally, if we can share code between both added chunks, that would be good too. Best way to do that might be to convert the hypotheticalBox into the correct writing-mode, but per comment 14 maybe that turns out to be complex.)
Comment on attachment 8635310 [details] [diff] [review]
Convert writing mode if necessary when getting bStart from hypothetical-box

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

::: layout/generic/nsHTMLReflowState.cpp
@@ +1527,5 @@
>      // Treat 'top' like 'static-position'
> +    if (hypotheticalBox.mWritingMode.GetBlockDir() == cbwm.GetBlockDir()) {
> +      offsets.BStart(cbwm) = hypotheticalBox.mBStart;
> +    } else {
> +      LogicalRect r(hypotheticalBox.mWritingMode,

If this code remains in the final patch here, it'd be nice to include a brief comment explaining what you're doing here (both for reviewing and for attempts to understand this code in the future).

Something like this (if this is correct):
 // We need the BStart edge (from the perspective of cbwm) of hypotheticalBox.

@@ +1529,5 @@
> +      offsets.BStart(cbwm) = hypotheticalBox.mBStart;
> +    } else {
> +      LogicalRect r(hypotheticalBox.mWritingMode,
> +                    hypotheticalBox.mIStart, hypotheticalBox.mBStart,
> +                    hypotheticalBox.mIEnd - hypotheticalBox.mIStart, 0);

Might be worth adding a convenience "hypotheticalBox.ISize()" method, to shorten this a bit and make it clearer what we're doing here.

Also: it looks like we're using 0 for the BSize of our LogicalRect here. Is that bad? In particular, suppose hypotheticalBox.mWritingMode and cbwm only differ by having a opposing block-directions (vertical-lr vs. vertical-rl).  Won't that make our conversion here break? Please include a reftest along those lines regardless, and a code-comment about the 0 usage here. If that reftest needs to be marked as 'fails' for now, which I think it might*, please file a followup bug.

*(For it to pass, I think we'd need to know hypotheticalBox.mBSize, which we don't store for some reason, probably because it requires reflow.)
(In reply to Daniel Holbert [:dholbert] from comment #16)
> Created attachment 8636152 [details]
> testcase 2, showing the static position in this scenario ("bbb" should be
> inside the pink box)
> 
> With the current patch applied, "bbb" is off to the right here for some
> reason. (And if I remove everything before the pink div, it's off to the
> left instead.)
> 
> It should be inside the pink div.

This testcase (and other examples like it) fail badly with current trunk code, too. Basically, this is demonstrating that we need a more extensive reworking of the hypotheticalBox code to actually get things right in various modes. The testcases from bug 1079151 clearly don't cover enough scenarios. :(
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #19)
> This testcase (and other examples like it) fail badly with current trunk
> code, too.

(Yup -- didn't mean to imply that this patch *breaks* this testcase. Just that the patch isn't sufficient to get us a working hypotheticalBox.)

> Basically, this is demonstrating that we need a more extensive
> reworking of the hypotheticalBox code to actually get things right in
> various modes. The testcases from bug 1079151 clearly don't cover enough
> scenarios. :(

Yeah. :-/ Should we fix that more robustly here, or in a followup?

If you want to spin off a followup & take a version of the current patch here, as a step in the right direction, I think I'd be OK with that, as long as the patch here includes a reftest which demonstrates that the math is going from incorrect to correct, in some simple orthogonal-flow scenario.

(with other nits in comment 15 - 18 addressed, too.)
I'm extending the scope of this bug to try and fix most of the incorrect rendering, not just avoid the assertion; this will involve a more extensive rewrite of CalculateHypotheticalBox and associated code, but it seems like the only reasonable way forward.
Summary: "ASSERTION: block direction mismatch" with writing-mode, position: fixed → "ASSERTION: block direction mismatch" and incorrect static position for positioned element with writing-mode and position: fixed or absolute
Here are a collection of testcases checking the static position we compute for an abs-pos element in orthogonal-direction scenarios, using an image with explicit size; an intrinsically-sized image; and a block (paragraph) element. In each case, the a, b and c testcases should all render the same; they differ only in which element is the containing block for absolute positioning, but as the abs-pos element has no offsets, it should remain at the hypothetical-box position we compute in all cases. Currently, all the (a) testcases fail (and assert) due to this bug; as a bonus, 6c also fails (though without asserting). I included the (b) and (c) testcases partly to demonstrate how the problem here depends on the relationship between writing modes of the absolute containing block and the placeholder's immediately containing block, and partly to ensure that fixing the (a) cases doesn't regress the others.
Attachment #8642986 - Flags: review?(dholbert)
And here's a patch that I think makes things substantially better here. This fixes all the above testcases except for 6a, which would require us to reflow the positioned block during CalculateHypotheticalBox in order to get its correct block-size (see the XXX left in the code); that seems like a good candidate for a followup.
Attachment #8642988 - Flags: review?(dholbert)
Comment on attachment 8642988 [details] [diff] [review]
Ensure hypothetical box has a writing mode with the same block direction as the absolute containing block

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

::: layout/generic/nsHTMLReflowState.h
@@ +941,5 @@
> +  // aInsideBoxSizing returns the part of the padding, border, and margin
> +  // in the aAxis dimension that goes inside the edge given by box-sizing;
> +  // aOutsideBoxSizing returns the rest.
> +  void CalculateBorderPaddingMargin(mozilla::LogicalAxis aAxis,
> +                                    nscoord aContainingBlockISize,

oops, s/ISize/Size/ here (as in the .cpp file)
Attachment #8635310 - Attachment is obsolete: true
Oops, I broke the patch with a bit of last-minute "cleanup", as tryserver (comment 24) so kindly pointed out. So here's a version that actually builds...
Attachment #8643006 - Flags: review?(dholbert)
Attachment #8642988 - Attachment is obsolete: true
Attachment #8642988 - Flags: review?(dholbert)
Comment on attachment 8642986 [details] [diff] [review]
Tests for hypothetical box computation (to determine static position of abspos element) where orthogonal writing modes are involved

r=me on the reftests. (Thanks for the a/b/c robustness!)

(You might also want to include the original testcase as a crashtest, but maybe the asserting reftests are similar enough to remove the need for it.)
Attachment #8642986 - Flags: review?(dholbert) → review+
Comment on attachment 8643006 [details] [diff] [review]
Ensure hypothetical box has a writing mode with the same block direction as the absolute containing block

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

Review notes, wave 1 / 2:

::: layout/generic/nsHTMLReflowState.cpp
@@ +1021,5 @@
>    if (state) {
>      WritingMode stateWM = state->GetWritingMode();
>      aCBIStartEdge =
>        state->ComputedLogicalBorderPadding().ConvertTo(wm, stateWM).IStart(wm);
> +    aCBSize = state->ComputedSize(wm);

(I was initially confused about why we're using |wm| instead of |stateWM| here. On further analysis, I think those variables are actually actually the same; filed bug 1191109 on consolidating them.)

@@ +1100,4 @@
>  
> +  nscoord borderStartEnd = (aAxis == eLogicalAxisInline)
> +    ? LogicalMargin(wm, mStyleBorder->GetComputedBorder()).IStartEnd(wm)
> +    : LogicalMargin(wm, mStyleBorder->GetComputedBorder()).BStartEnd(wm);

Given that we already inspected aAxis to figure out startSide and endSide, the check on aAxis here feels redundant...  And the physical-margin --> LogicalMargin conversion feels unnecessary as well.

Seems like it'd be a bit more direct to apply the physical sides (which we just looked up) to the physical margin, something like the following:
  nsMargin styleBorder = mStyleBorder->GetComputedBorder();
  nscoord borderStartEnd =
    styleBorder->GetSide(startSide) + styleBorder->GetSide(endSide);

@@ +1108,5 @@
>    nsMargin stylePadding;
>    if (mStylePadding->GetPadding(stylePadding)) {
> +    paddingStartEnd = (aAxis == eLogicalAxisInline)
> +      ? LogicalMargin(wm, stylePadding).IStartEnd(wm)
> +      : LogicalMargin(wm, stylePadding).BStartEnd(wm);

Same here.

@@ +1126,5 @@
>    nsMargin styleMargin;
>    if (mStyleMargin->GetMargin(styleMargin)) {
> +    marginStartEnd = (aAxis == eLogicalAxisInline)
> +      ? LogicalMargin(wm, styleMargin).IStartEnd(wm)
> +      : LogicalMargin(wm, styleMargin).BStartEnd(wm);

Same here.

@@ +1151,4 @@
>    }
>  
>    nscoord outside =
> +    paddingStartEnd + borderStartEnd + marginStartEnd;

Unwrap the assignment here -- before your code, we had this wrapped to 2 lines, but we don't need to anymore.

In case splinter only prints out one line of context, the statement I'm talking about is:
  nscoord outside =
    paddingStartEnd + borderStartEnd + marginStartEnd;

...which can just become:
  nscoord outside = paddingStartEnd + borderStartEnd + marginStartEnd;

@@ +1198,1 @@
>  // cbrs->frame is the actual containing block

(Per end of comment 15, I think you still need to remove or adjust this final sentence here. I think the point of this sentence is to define what is meant by "actual containing block" earlier in the comment, but you're removing that usage from earlier in the comment, so this statement/definition seems vestigial now. I believe cbrs->frame is *really* what you're calling the "absolute containing block", as discussed in comment 15.)

@@ +1216,4 @@
>    nsIFrame* containingBlock =
>      GetHypotheticalBoxContainer(aPlaceholderFrame, blockIStartContentEdge,
> +                                blockContentSize);
> +  // Now blockContentSize is in containingBlock's writing mode

nit: add period at end of comment.
Comment on attachment 8643006 [details] [diff] [review]
Ensure hypothetical box has a writing mode with the same block direction as the absolute containing block

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

wave 2/2 of review comments below. r=me, with "wave 1" and "wave 2" below addressed.

I'm also happy to take another look at this, if you change the logic enough that you think it's worth another round of review, though I'm off the grid for the next week (returning Tuesday).

::: layout/generic/nsHTMLReflowState.cpp
@@ +1449,5 @@
>    aHypotheticalBox.mBStart -= border.BStart(wm);
> +
> +  if (cbwm.GetBlockDir() != wm.GetBlockDir()) {
> +    nscoord insideBoxSizing, outsideBoxSizing;
> +    CalculateBorderPaddingMargin(eLogicalAxisBlock, blockContentSize.BSize(wm),

This new section needs a comment explaining, at a high level, what's going on here.

e.g. something like:
"The abspos element's WM is orthogonal to its absolute containing block, so we need to measure the abspos element in its own block axis, to determine its inline start/end in its containing block's WM."

(Hopefully/maybe you can come up with something clearer. :))

@@ +1457,5 @@
> +    bool isAutoBSize = styleBSize.GetUnit() == eStyleUnit_Auto;
> +    if (isAutoBSize && NS_FRAME_IS_REPLACED(mFrameType) && knowIntrinsicSize) {
> +      boxBSize = LogicalSize(wm, intrinsicSize).BSize(wm) +
> +                 outsideBoxSizing + insideBoxSizing;
> +    } else if (!isAutoBSize) {

Can we make this logic closer to the logic in the code above that does the same thing (the pre-existing code that sets boxISize instead of boxBSize)? Or does it need to differ for some reason?

In an ideal world, we'd want to share this logic with the code that sets boxISize -- but if that's too messy [and I suspect it might be], the logic should be as similar as possible.

For example -- right now it looks like they differ (at least structurally) in the ordering of conditions here (that's trivial), and they also differ in behavior when knowIntrinsicSize is false but the other conditions are true (not sure if that's trivial). Is that latter difference meaningful? If so, we should probably call attention to it with a comment; if not, we should probably structure the code the same way so that it's clearer we're being consistent.

(Side note: it looks like <iframe> will produce a frame that exercises this scenario -- it'll return true from NS_FRAME_IS_REPLACED(), but knowIntrinsicSize will be false. It's probably worth adding some additional reftests with an <iframe>, to test this code-path. Copypasting your intrinsically-sized <img> tests and swapping in <iframe> would probably be fine.)

@@ +1464,5 @@
> +                 insideBoxSizing + outsideBoxSizing;
> +    } else {
> +      // XXX TODO:
> +      // Figure out how to get the correct boxBSize here (reflow?)
> +      boxBSize = 0;

Please file a followup on this, and mention its bug number here & in reftest.list for the test that's still failing due to this.

@@ +1477,5 @@
> +    aHypotheticalBox.mIStart = origin.I(cbwm);
> +    aHypotheticalBox.mIEnd = aHypotheticalBox.mIStart +
> +                             boxSize.ConvertTo(cbwm, wm).ISize(cbwm);
> +#ifdef DEBUG
> +    aHypotheticalBox.mIEndIsExact = false; // it may be fake

I was going to ask for a slightly-clearer comment here, but I think this member-variable just wants to be removed; I filed bug 1191185 on that. :)
Attachment #8643006 - Flags: review?(dholbert) → review+
Blocks: 1191801
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/27eab13d3cf2e7eeb174a3eb9798ed6df41f26c9
changeset:  27eab13d3cf2e7eeb174a3eb9798ed6df41f26c9
user:       Jonathan Kew <jkew@mozilla.com>
date:       Thu Aug 06 15:44:48 2015 +0100
description:
Bug 1183431 - Tests for hypothetical box computation (to determine static position of abspos element) where orthogonal writing modes are involved. r=dholbert

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/94831690f525b1eced48bca0ce233045cb72de98
changeset:  94831690f525b1eced48bca0ce233045cb72de98
user:       Jonathan Kew <jkew@mozilla.com>
date:       Thu Aug 06 15:44:50 2015 +0100
description:
Bug 1183431 - Ensure hypothetical box has a writing mode with the same block direction as the absolute containing block. r=dholbert
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/74ecf0f57a56819ac0f443fedf0915952db8c658
changeset:  74ecf0f57a56819ac0f443fedf0915952db8c658
user:       Jonathan Kew <jkew@mozilla.com>
date:       Thu Aug 06 16:25:07 2015 +0100
description:
Bug 1183431 followup - Add the test files that I failed to "hg add" when updating the reftest patch, because it's going to burn the CLOSED TREE.
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/1639af64e372ff6a398f5418d32967521239ad81
changeset:  1639af64e372ff6a398f5418d32967521239ad81
user:       Jonathan Kew <jkew@mozilla.com>
date:       Thu Aug 06 16:36:50 2015 +0100
description:
Bug 1183431 followup - Crashtest no longer asserts, so mark it appropriately on the CLOSED TREE.
(In reply to Daniel Holbert [:dholbert] (out 8/5-8/10) from comment #28)
> Comment on attachment 8642986 [details] [diff] [review]
> Tests for hypothetical box computation (to determine static position of
> abspos element) where orthogonal writing modes are involved
> 
> r=me on the reftests. (Thanks for the a/b/c robustness!)
> 
> (You might also want to include the original testcase as a crashtest, but
> maybe the asserting reftests are similar enough to remove the need for it.)

I added back the crashtest, and also added a set of testcases using <iframe> in place of <img>, as suggested in comment 30. The <iframe> test highlighted that we're probably sizing it wrongly, so I filed bug 1191855 on that, and bug 1191801 on the tests that still fail because we need to reflow the box to find its block-size.

(In reply to Daniel Holbert [:dholbert] (out 8/5-8/10) from comment #29)
> ::: layout/generic/nsHTMLReflowState.cpp
> @@ +1021,5 @@
> >    if (state) {
> >      WritingMode stateWM = state->GetWritingMode();
> >      aCBIStartEdge =
> >        state->ComputedLogicalBorderPadding().ConvertTo(wm, stateWM).IStart(wm);
> > +    aCBSize = state->ComputedSize(wm);
> 
> (I was initially confused about why we're using |wm| instead of |stateWM|
> here. On further analysis, I think those variables are actually actually the
> same; filed bug 1191109 on consolidating them.)

I remember doing this in some earlier version of the patch, actually, but clearly it got lost among the various approaches I was trying here. So we'll clean this up in your followup bug.
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/4a90edbfe8ff9665b26b2ec3950f30a28b9da7ba
changeset:  4a90edbfe8ff9665b26b2ec3950f30a28b9da7ba
user:       Jonathan Kew <jkew@mozilla.com>
date:       Thu Aug 06 17:32:20 2015 +0100
description:
Backout changesets 1639af64e372, 74ecf0f57a56, 94831690f525, 27eab13d3cf2 (bug 1183431) for Android-specific failure in reftest 1183431-orthogonal-modes-5a.html.
(In reply to Jonathan Kew (:jfkthame) from comment #35)
> Backout changesets 1639af64e372, 74ecf0f57a56, 94831690f525, 27eab13d3cf2
> (bug 1183431) for Android-specific failure in reftest
> 1183431-orthogonal-modes-5a.html.

Sigh... backed out, because apparently on Android we don't know the intrinsic size of an <img> frame, or at least not early enough for the reftest to pass -- in test 5a, we end up using a (fake) boxBSize of zero instead. I wonder if this is a result of not providing an actual image resource, but just using the "missing-image" rendering; pushed a try job where the <img> loads a real .png, to see if that works better.
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f46ed83467c00cbd7b8f1a2bcee1e67df5da6e
changeset:  b1f46ed83467c00cbd7b8f1a2bcee1e67df5da6e
user:       Jonathan Kew <jkew@mozilla.com>
date:       Thu Aug 06 21:00:45 2015 +0100
description:
Bug 1183431 - Tests for hypothetical box computation (to determine static position of abspos element) where orthogonal writing modes are involved. r=dholbert

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/2debc63507da51f9f884a0e6e7c3bd9bac254dc3
changeset:  2debc63507da51f9f884a0e6e7c3bd9bac254dc3
user:       Jonathan Kew <jkew@mozilla.com>
date:       Thu Aug 06 21:01:31 2015 +0100
description:
Bug 1183431 - Ensure hypothetical box has a writing mode with the same block direction as the absolute containing block. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/b1f46ed83467
https://hg.mozilla.org/mozilla-central/rev/2debc63507da
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(In reply to Jonathan Kew (:jfkthame) from comment #37)
> Sigh... backed out, because apparently on Android we don't know the
> intrinsic size of an <img> frame, or at least not early enough for the
> reftest to pass

Hmm, this sounds like an incremental layout bug.  I seem to recall that all reftests are loaded over HTTP (not as file:///) on android, which probably means image loads end up being more asynchronous.  That's probably why we don't know the intrinsic size of the <img> frame up-front there. BUT, we should end up redoing layout after the <img>'s data has loaded (or after we've determined that it's a broken-image in this case), though.

Could you file a bug on that <img> misrendering, if it's not already covered somewhere?
It turns out this can be reproduced (erratically) on desktop, too. Filed bug 1197153 about it.
Flags: needinfo?(jfkthame)
Depends on: 1227410
You need to log in before you can comment on or make changes to this bug.