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

VERIFIED FIXED in Firefox 45

Status

()

Core
Layout: Block and Inline
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Oriol, Assigned: jfkthame)

Tracking

({regression, testcase})

36 Branch
mozilla47
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed, firefox46 verified, firefox47 verified)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8712349 [details]
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
(Reporter)

Updated

3 years ago
Blocks: 1144501
Keywords: regression, testcase

Updated

3 years ago
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → 40 Branch

Updated

3 years ago
Flags: needinfo?(jfkthame)
(Reporter)

Comment 1

3 years ago
Created attachment 8714008 [details]
testcase2
Attachment #8712349 - Attachment is obsolete: true
(Reporter)

Comment 2

3 years ago
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.
Blocks: 1083848
Summary: Problem when mixing writing-mode, margin, height and float → Problem when reflowing orthogonal block taller than its container
(Reporter)

Comment 3

3 years ago
Oops, sorry, I meant bug 1103613.
Blocks: 1103613
No longer blocks: 1083848

Updated

3 years ago
Version: 40 Branch → 36 Branch
(Reporter)

Comment 4

3 years ago
Created attachment 8714131 [details]
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.
(Reporter)

Comment 5

3 years ago
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.
(Reporter)

Comment 6

3 years ago
Created attachment 8714379 [details] [diff] [review]
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.
Attachment #8714379 - Flags: review?(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?
Attachment #8714379 - Flags: review?(bzbarsky) → review?(jfkthame)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
(Assignee)

Comment 9

3 years ago
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.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 10

3 years ago
Created attachment 8714889 [details] [diff] [review]
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.
(Assignee)

Comment 11

3 years ago
Created attachment 8714891 [details] [diff] [review]
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.
(Assignee)

Comment 12

3 years ago
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?
Attachment #8714889 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Attachment #8714891 - Flags: review?(dholbert)
(Reporter)

Comment 13

3 years ago
Created attachment 8714966 [details]
Multiple testcases

Maybe something like this can be used as a reftest.
Attachment #8714008 - Attachment is obsolete: true
Attachment #8714131 - Attachment is obsolete: true
Attachment #8714891 - Flags: review?(dholbert) → review+
(Reporter)

Updated

3 years ago
Summary: Problem when reflowing orthogonal block taller than its container → Problem when float with vertical writing-mode is taller than its container
(Assignee)

Comment 14

3 years ago
Looks good, thanks!
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.
Attachment #8714889 - Flags: review?(dholbert) → review+
(Reporter)

Comment 16

3 years ago
Created attachment 8715015 [details] [diff] [review]
Reftest for floats overflowing container, diverse writing-mode
Attachment #8715015 - Flags: review?(jfkthame)
(Assignee)

Comment 17

3 years ago
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!
Attachment #8714379 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 18

3 years ago
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.
Attachment #8715015 - Flags: review?(jfkthame) → review+
(Assignee)

Updated

3 years ago
Assignee: nobody → oriol-bugzilla
(Assignee)

Comment 20

3 years ago
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

3 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 22

3 years ago
Created attachment 8717837 [details] [diff] [review]
(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.
(Assignee)

Updated

3 years ago
Assignee: oriol-bugzilla → jfkthame
(Assignee)

Comment 23

3 years ago
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
Attachment #8717837 - Flags: approval-mozilla-beta?
Attachment #8717837 - Flags: approval-mozilla-aurora?
status-firefox45: --- → affected
status-firefox46: --- → affected
Jonathan, if I understand correctly, the regression is not recent. Can this ride the train from 46? Thanks
Flags: needinfo?(jfkthame)
(Assignee)

Comment 25

3 years ago
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.
Flags: needinfo?(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

Ok, merci.
Taking it then, should be in 45 beta 5.
Attachment #8717837 - Flags: approval-mozilla-beta?
Attachment #8717837 - Flags: approval-mozilla-beta+
Attachment #8717837 - Flags: approval-mozilla-aurora?
Attachment #8717837 - Flags: approval-mozilla-aurora+

Comment 27

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/6b2cd1fe6cd7
status-firefox46: affected → fixed

Comment 28

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/3a1183062d0d
status-firefox45: affected → fixed
QA Whiteboard: [good first verify]
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
QA Whiteboard: [good first verify] → [good first verify][testday-20160304]
status-firefox46: fixed → verified
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!
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify][testday-20160304] → [good first verify][testday-20160304][bugday-20160309]
status-firefox47: fixed → verified
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1225586
You need to log in before you can comment on or make changes to this bug.