1243125 - Problem when float with vertical writing-mode is taller than its container

Status: VERIFIED FIXED

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

Description

[Oriol Brufau [:Oriol]]

Attachment: testcase

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160126030244

Steps to reproduce:

Run the testcase.


Actual results:

There is an empty document with a long scrollbar.


Expected results:

There should be a document saying "Hello World".

Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9268463e&tochange=e73ef97e

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: testcase2

Comment 2

[Oriol Brufau [:Oriol]]

OK, I have investigated a bit. The problem with the first testcase is

 - There is a container with height:200px
 - It contains an orthogonal element with height:200px and some margin-top
 - Bug 1144501 made that margin was taken into account. Then height+margin > containerHeight

That's the root of the problem.

Just using greater height instead of margin reproduces the problem on older builds.

Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=74edfbf0e6a3&tochange=ced1402861b8

I suspect bug 1083848, also by :jfkthame.

Comment 3

[Oriol Brufau [:Oriol]]

Oops, sorry, I meant bug 1103613.

Comment 4

[Oriol Brufau [:Oriol]]

Attachment: testcase3

The problem seems to happen when the orthogonal flow has a definite height greater than the constraint given by AvailableISize.

Bug 1103613 made orthogonal flows be constrained by the height of their parent. Before that the constraint was the height of the window.

Then this third testcase uses height:101vh to reproduce the problem on older builds.

Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b4fbeba78a7d&tochange=6ce1b906c690

The cause seems bug 1097128 - when vertical writing-mode landed.

Comment 5

[Oriol Brufau [:Oriol]]

OK, I think I have fixed it.

In layout/generic/nsHTMLReflowState.cpp, there is

> void
> nsHTMLReflowState::SetTruncated(const nsHTMLReflowMetrics& aMetrics,
>                                 nsReflowStatus* aStatus) const
> {
>   if (AvailableHeight() != NS_UNCONSTRAINEDSIZE &&
>       AvailableHeight() < aMetrics.Height() &&
>       !mFlags.mIsTopOfPage) {
>     *aStatus |= NS_FRAME_TRUNCATED;
>   } else {
>     *aStatus &= ~NS_FRAME_TRUNCATED;
>   }
> }

Using block sizes instead of height seems to work:

> void
> nsHTMLReflowState::SetTruncated(const nsHTMLReflowMetrics& aMetrics,
>                                 nsReflowStatus* aStatus) const
> {
>   if (AvailableBSize() != NS_UNCONSTRAINEDSIZE &&
>       AvailableBSize() < aMetrics.BSize(GetWritingMode()) &&
>       !mFlags.mIsTopOfPage) {
>     *aStatus |= NS_FRAME_TRUNCATED;
>   } else {
>     *aStatus &= ~NS_FRAME_TRUNCATED;
>   }
> }

I have no idea whether this can break other things, though.

Comment 6

[Oriol Brufau [:Oriol]]

Attachment: Check block size instead of height when detecting truncated frames

This is my first patch, so let me know if I did something wrong.

Requesting bzbarsky because it's the suggested reviewer with smallest queue.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8714379 [details] [diff] [review]
Check block size instead of height when detecting truncated frames

Thank you for the patch!

I've not been following the writing-modes work very well, so I'm not a good reviewer for this patch; it would take me a while to figure out whether it makes sense or not.

Jonathan, can you take a look, please?

Comment 8

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

Your analysis of where the problem is looks good.

The interesting question is whose writing-mode we should care about in this code.  It seems like maybe we want the writing-mode of the thing that's paginating (the root element for printing, the element with column-* properties for multicol) for deciding whether we paginate when content is too tall or when content is too wide.  It's possible we want to not paginate at all inside of an orthogonal flow.  But the spec might say something different.

Maybe the spec says something here?  And I'm curious what Jonathan thinks.

Comment 9

[Jonathan Kew [:jfkthame]]

It certainly looks like this code needs an update for logical directions; it can't be right to test (physical) height here in vertical writing modes. So far, so good.

(I pushed a try run with attachment 8714379 [details] [diff] [review]:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7be6b7c8ca5a

which showed that it results in a number of "writing-mode mismatch" assertions. However, looking into this, it appears that they are spurious: the checks in the ISize() and BSize() accessors of nsHTMLReflowMetrics are overly strict, and are asserting due to bidi mismatches that are irrelevant to the ISize/BSize values. So I'll post an additional patch to relax those checks.)

Regarding pagination, as a rule we aim to avoid paginating across orthogonal-flow boundaries (see code at the end of nsHTMLReflowState::Init); we ensure there is an unconstrained dimension when reflowing, so that the reflow will complete. AFAIR, I don't think the specs (either Writing Modes or Fragmentation) really make it clear what should be done here, but at least for an initial implementation it seemed best to avoid the issue.

So in the case where the writing mode of the reflow-metrics we've been given is orthogonal to the reflow-state's own mode (which would make the BSize check bogus), we shouldn't ever need to set the truncated flag anyway and we can skip the check altogether.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - We never need to set NS_FRAME_TRUNCATED for orthogonal flows

So I think Oriol's patch is basically right, with this addition to avoid potentially bogus checks for orthogonal cases.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: patch 0 - Relax overly-harsh writing mode assertions in nsReflowMetrics size accessors

With patch 2, we'll no longer hit those assertions on various bidi testcases; but we should relax them anyway as they're somewhat bogus.

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 8714889 [details] [diff] [review]
patch 2 - We never need to set NS_FRAME_TRUNCATED for orthogonal flows

:dholbert, any chance you could have a look at this, as dbaron is currently marked as busy?

Comment 13

[Oriol Brufau [:Oriol]]

Attachment: Multiple testcases

Maybe something like this can be used as a reftest.

Comment 14

[Jonathan Kew [:jfkthame]]

Looks good, thanks!

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8714889 [details] [diff] [review]
patch 2 - We never need to set NS_FRAME_TRUNCATED for orthogonal flows

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

r=me with nits addressed as you see fit.

First, a commit-message nit:
> We never need to set NS_FRAME_TRUNCATED for orthogonal flows.

Right now, this commit-message is phrased as an observation, rather than as a statement about what the patch is actually doing.

Maybe change this to be more descriptive, like
  Don't ever set NS_FRAME_TRUNCATED for orthogonal flows.

::: layout/generic/nsHTMLReflowState.cpp
@@ +2925,5 @@
>  nsHTMLReflowState::SetTruncated(const nsHTMLReflowMetrics& aMetrics,
>                                  nsReflowStatus* aStatus) const
>  {
> +  const WritingMode wm = aMetrics.GetWritingMode();
> +  if (GetWritingMode().IsOrthogonalTo(wm)) {

Let's give this a more meaningful name than "wm", since there are multiple writing modes in play here (and "wm" belongs to somebody else).

Maybe "containerWM"?  (I think that's what it represents; it comes from aMetrics, which is set up by our container.)

@@ +2933,2 @@
>    } else {
> +    if (AvailableBSize() != NS_UNCONSTRAINEDSIZE &&

Maybe just "else if"  instead of "else { if " here.

I find that clearer, and it means you can avoid having to reindent all of the old code here, I think.

Comment 16

[Oriol Brufau [:Oriol]]

Attachment: Reftest for floats overflowing container, diverse writing-mode

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 8714379 [details] [diff] [review]
Check block size instead of height when detecting truncated frames

r=me, in conjunction with the additional fixes here. Thanks for identifying this one!

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8715015 [details] [diff] [review]
Reftest for floats overflowing container, diverse writing-mode

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

LGTM, thanks again.

::: layout/reftests/writing-mode/1243125-1-floats-overflowing-ref.html
@@ +18,5 @@
> +
> +  <div></div>
> +  <div></div>
> +  <div></div>
> +  

One trivial nit: we prefer to avoid trailing whitespace. I'll remove this locally before pushing it.

Comment 19

[Jonathan Kew [:jfkthame]]

Try run with reftest: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7847cf1f64f3

Comment 20

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1eeeeb1cce5179370d8f7e40d93779e3fcb3ce
Bug 1243125 - patch 0 - Relax overly-harsh writing mode assertions in nsReflowMetrics size accessors. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/c48856457f889eb00272a15b2c10151207088bd8
Bug 1243125 - patch 1 - Check block size instead of height when detecting truncated frames. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bd6ad5694118f3a3a1bb256e51a09f1ed1819d8
Bug 1243125 - patch 2 - Don't ever set NS_FRAME_TRUNCATED for orthogonal flows. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f4756d4bab615ade4e575b2f3343772a926abd0
Bug 1243125 - Reftest for floats overflowing container, with diverse writing-modes. r=jfkthame

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c1eeeeb1cce
https://hg.mozilla.org/mozilla-central/rev/c48856457f88
https://hg.mozilla.org/mozilla-central/rev/4bd6ad569411
https://hg.mozilla.org/mozilla-central/rev/6f4756d4bab6

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: (Rollup, including reftest) - Properly handle vertical writing mode and orthogonal flows when checking for truncated frames in reflow

In bug 1243623 comment 10, Roc suggests that we should backport the changes here. This is a rollup of the changesets that landed on central (including reftests as well as the code patches), ready to apply on aurora and beta.

Comment 23

[Jonathan Kew [:jfkthame]]

Comment on attachment 8717837 [details] [diff] [review]
(Rollup, including reftest) - Properly handle vertical writing mode and orthogonal flows when checking for truncated frames in reflow

Approval Request Comment
[Feature/regressing bug #]: bug 1103613, bug 1144501

[User impact if declined]: incorrect layout with orthogonal flows; bug 1243623 suggests this can also result in potential crashes (or other issues?)

[Describe test coverage new/current, TreeHerder]: includes reftests

[Risks and why]: minimal risk, simple code change to check block-size instead of height

[String/UUID change made/needed]: n/a

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Jonathan, if I understand correctly, the regression is not recent. Can this ride the train from 46? Thanks

Comment 25

[Jonathan Kew [:jfkthame]]

Normally, I'd say yes; but given the concerns Roc raises in bug 1243623 comment 10 and 14 (and which this resolves), I think it might be wiser to uplift to 45.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8717837 [details] [diff] [review]
(Rollup, including reftest) - Properly handle vertical writing mode and orthogonal flows when checking for truncated frames in reflow

Ok, merci.
Taking it then, should be in 45 beta 5.

Comment 27

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b2cd1fe6cd7

Comment 28

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

https://hg.mozilla.org/releases/mozilla-beta/rev/3a1183062d0d

Comment 29

[Khalid Syfullah Zaman [:khalid32]]

I've reproduced this bug on Nightly 47.0a1(2016-01-26) on Ubuntu 14.04 LTS, 32-bit!

The bug's fix is now verified on Latest Developer Edition 46.0a2!

Build ID: 20160303004038
User Agent: Mozilla/5.0 (X11; Linux i686; rv:46.0) Gecko/20100101 Firefox/46.0

Comment 30

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 47.0a1 (Build ID: 20160126030244) on 
windows 8.1, 64-bit with the instructions from comment 0.

Verified as fixed with Firefox Aurora 46.0a2 (Build ID: 20160307004010)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0

Verified as fixed with Firefox Nightly 47.0a1 (Build ID: 20160307030208)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0

As this bug is also verified on Ubuntu (Comment 29), I am marking this as verified!