Closed Bug 1114329 Opened 9 years ago Closed 9 years ago

regression in positioning of floats in RTL pages (The left shows the bottom of the page)

Categories

(Core :: Layout: Floats, defect)

36 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox35 --- unaffected
firefox36 - wontfix
firefox37 + fixed
firefox38 --- fixed
firefox39 --- fixed
b2g-v2.2 --- fixed
b2g-master --- affected

People

(Reporter: over68, Assigned: smontagu, NeedInfo)

References

Details

(Keywords: regression, testcase)

Attachments

(9 files, 3 obsolete files)

1.33 KB, text/html
Details
3.71 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
7.57 KB, patch
jfkthame
: review-
Details | Diff | Splinter Review
696 bytes, text/html
Details
16.03 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
4.96 KB, patch
Details | Diff | Splinter Review
21.51 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
87.26 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
67.46 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
This problem occurs since the version 36.0a1 (2014-10-23).

https://hg.mozilla.org/mozilla-central/rev/88adcf8fef83
[Tracking Requested - why for this release]: regression
Status: UNCONFIRMED → NEW
Ever confirmed: true
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=506226a2e6c0&tochange=f1d7da5ff1ed

Last Good: 1de2c87f94e5
First Bad: 2a2316981708

Triggered by:
	2a2316981708	Simon Montagu — Bug 1062963 patch 3: make nsFloatManager's origin a LogicalPoint, adapt GetFlowAreas, AddFloats, ClearFloats, etc. to use it and make nsFloatManager region functions work with logical region. r=jfkthame
Flags: needinfo?(smontagu)
Attached file reduced testcase
Component: General → Layout: Floats
This is fixed by the patch in bug 1113526
Depends on: 1113526
Flags: needinfo?(smontagu)
Attached patch Reftest based on the testcase (obsolete) — Splinter Review
Assignee: nobody → smontagu
Attachment #8540209 - Flags: review?(jfkthame)
Comment on attachment 8540209 [details] [diff] [review]
Reftest based on the testcase

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

::: layout/reftests/writing-mode/1114329.html
@@ +20,5 @@
> +      <script type="text/javascript">
> +        setTimeout(function(){
> +          document.getElementById("image").removeAttribute("hidden");
> +          document.documentElement.removeAttribute("class");
> +        },1000);

The setTimeout here is a bit worrying.... this looks like a recipe for an intermittent test. Can we avoid that, perhaps adding something like document.documentElement.offsetHeight to force a layout flush before we remove the "hidden" attribute, and/or calling the script from an onload() handler instead of putting it directly inline?
Comment on attachment 8540209 [details] [diff] [review]
Reftest based on the testcase

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

Also, this test should go in layout/reftests/floats, not the writing-mode directory.
Here's an updated version of the reftest that appears to work reliably for me in local testing, at least, and avoids the use of the timeout. Note also the added <img> in the main content area; without this, I was getting an intermittent failure in my debug build, when the reftest image was apparently captured before the broken-image icon was fully decoded and ready to draw. The added image is intended to trigger earlier loading of that icon and hence avoid this problem.
Attachment #8540696 - Flags: review?(smontagu)
Assignee: smontagu → jfkthame
Status: NEW → ASSIGNED
Attachment #8540696 - Attachment description: Reftest for → updated version of reftest based on the testcase
Please make sure whether the patch in bug 1113526 is to fix the bug in page https://dl.dropboxusercontent.com/u/95157096/85f61cf7/olenn2imx2.html
(In reply to blinky from comment #10)
> Please make sure whether the patch in bug 1113526 is to fix the bug in page
> https://dl.dropboxusercontent.com/u/95157096/85f61cf7/olenn2imx2.html

No, unfortunately it doesn't, although it does fix the reduced testcase in attachment 8539797 [details]
No longer depends on: 1113526
Comment on attachment 8540696 [details] [diff] [review]
updated version of reftest based on the testcase

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

Yes, this is clearly an improvement
Attachment #8540696 - Flags: review?(smontagu) → review+
Attachment #8540209 - Attachment is obsolete: true
Attachment #8540209 - Flags: review?(jfkthame)
https://hg.mozilla.org/mozilla-central/rev/bf55c6f6424d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
See comment 10 and bug 1113526 comment 11: this is not fixed. I suppose I should have marked it leave-open when checking in the testcase.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Jonathan, Simon: do you have plans for this? There are not much time left. We will start the build of 36 beta 5 today.
Flags: needinfo?(smontagu)
Flags: needinfo?(jfkthame)
I don't have anything -- Simon, you have any possible leads on this?

(Unassigning, as I'm not actively working on this; sorry, the assignee setting was an accidental side-effect of using bzexport to upload the reftest in comment 9.)
Assignee: jfkthame → nobody
Flags: needinfo?(jfkthame)
This regression looks like an edge case to me. (Simon: correct me if you disagree.) We don't need to rush a fix into beta 5 but please keep working on it.
OK. So, I am going to untrack it for 36. Don't hesitate to provide a fix for 37
The problem here occurs because items are added to the float manager with logical coordinates that were computed for different container widths, so that by the time we call GetFlowArea the rects are not correct when interpreted according to the final container width. We can fix this by keeping track of the container width when AddFloat is called, and checking this value when we later look at the stored rects. This fixes the example here, where we're currently getting an incorrect FlowArea for the left column, and concluding that the content doesn't quite fit. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59880beb7048.
Attachment #8559106 - Flags: review?(smontagu)
Comment on attachment 8559106 [details] [diff] [review]
Float manager needs to keep track of the container-width associated with logical coordinates in RTL modes

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

Yes, that looks right.
Attachment #8559106 - Flags: review?(smontagu) → review+
Tryserver says otherwise. Investigating reftest oranges...
Comment on attachment 8559106 [details] [diff] [review]
Float manager needs to keep track of the container-width associated with logical coordinates in RTL modes

Marking r-, as this doesn't work properly after all.
Attachment #8559106 - Flags: review+ → review-
Here's a reduced testcase that I think exhibits behavior related to the left-column problem seen on the http://arabic.sputniknews.com site. In this testcase, the yellow block is initially displayed in the correct position at the top of the page, but after changing its 'float' property to 'none' and then back to 'left', it moves below the gray block. It shouldn't do that.

A couple of key elements of this testcase appear to be the padding-right that's added to the <body> (and exceeds the 5px gap that I've left between the two colored blocks), and the presence of the extra <div> that wraps the gray float:right block.

Note that (just as on sputniknews.com), zooming the page in and out will cause the yellow block to pop back to its correct position.
(In reply to Jet Villegas (:jet) from comment #17)
> This regression looks like an edge case to me. (Simon: correct me if you
> disagree.) We don't need to rush a fix into beta 5 but please keep working
> on it.

Jet - Jonathan unassigned himself from this bug in comment 16. Can you find another owner?
Flags: needinfo?(bugs)
Assignee: nobody → smontagu
Flags: needinfo?(bugs)
Simon, I'll try to catch you on IRC to discuss tomorrow.... note that although I don't have a usable patch here, I have been looking at it some more, and I do have some additional testcases that may help us move forward.
Here are a collection of reftests, all of which fail with current trunk code (although they pass in Firefox 35).
Attachment #8565657 - Flags: review?(smontagu)
Looking at the testcases gave me an idea which I have begun pursuing: I think that instead of the float manager doing all its calculations in the caller's writing mode, it should convert the coordinates passed to it and do its calculations in its own writing mode.

This makes some code a little more involved, but mostly simplifies things. My current w-i-p patch makes us pass 12 out of the 16 tests in attachment 8565657 [details] [diff] [review], though it doesn't help with attachment 8564113 [details].
Flags: needinfo?(smontagu)
(In reply to Simon Montagu :smontagu from comment #27)
> Looking at the testcases gave me an idea which I have begun pursuing: I
> think that instead of the float manager doing all its calculations in the
> caller's writing mode, it should convert the coordinates passed to it and do
> its calculations in its own writing mode.

That seems reasonable. Another thing I've done is to add a container-width to the float manager, which is set when it is created (getting it from nsHTMLReflowState::GetContainingBlockContentWidth in nsAutoFloatManager::CreateFloatManager). This seems necessary if it's going to handle AddFloat callers that have various container-widths associated with the coords they're passing.

(I'm still getting lost somewhere among the various coordinate systems, though.)
Updated the reference files for these testcases to avoid any use of floats, so they should be more stable across float-manager patches.
Attachment #8565916 - Flags: review?(smontagu)
Updated the reference files for these testcases to avoid any use of floats, so they should be more stable across float-manager patches.
Attachment #8565917 - Flags: review?(smontagu)
Attachment #8565657 - Attachment is obsolete: true
Attachment #8565657 - Flags: review?(smontagu)
Attachment #8565916 - Attachment is obsolete: true
Attachment #8565916 - Flags: review?(smontagu)
Comment on attachment 8565917 [details] [diff] [review]
Reftests for floats within blocks of varying width and directionality

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

FTR, with this version, my patch mentioned above passes only 6 of these tests
Attachment #8565917 - Flags: review?(smontagu) → review+
Summary: The left shows the bottom of the page → regression in positioning of floats in RTL pages (The left shows the bottom of the page)
(In reply to Simon Montagu :smontagu from comment #27)
> Looking at the testcases gave me an idea which I have begun pursuing: I
> think that instead of the float manager doing all its calculations in the
> caller's writing mode, it should convert the coordinates passed to it and do
> its calculations in its own writing mode.
> 
> This makes some code a little more involved, but mostly simplifies things.
> My current w-i-p patch makes us pass 12 out of the 16 tests in attachment
> 8565657 [details] [diff] [review], though it doesn't help with attachment
> 8564113 [details].

I think this probably seems like a good idea.  Really, the float manager only cares about writing-mode to distinguish horizontal and vertical; left and right floats don't change when the inline-direction changes.

The current code to Translate and Untranslate the float manager is clearly broken.  The point of translation is to change the space manager's coordinate space into that of the block that's currently using it.  At the very least that requires:

(In reply to Jonathan Kew (:jfkthame) from comment #28)
> That seems reasonable. Another thing I've done is to add a container-width
> to the float manager, which is set when it is created (getting it from
> nsHTMLReflowState::GetContainingBlockContentWidth in
> nsAutoFloatManager::CreateFloatManager). This seems necessary if it's going
> to handle AddFloat callers that have various container-widths associated
> with the coords they're passing.

The current use of 0 width seems like the source of a number of the bugs in jfkthame's reftests.


Any chance you could share any of this work-in-progress?

Also, it seems like the reftests may as well land, no?


Some other things I've noticed and patched locally are:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/6e75416cc3c8/dont-mix-up-bend
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/6e75416cc3c8/float-manager-wm-asserts
Flags: needinfo?(smontagu)
Flags: needinfo?(jfkthame)
(In reply to David Baron [:dbaron] (UTC-8) from comment #32)
> The current code to Translate and Untranslate the float manager is clearly
> broken.  The point of translation is to change the space manager's
> coordinate space into that of the block that's currently using it.

To explain this in a little more detail:  the translation on the float manager is much like the translation on a graphics context.  The float manager initially manages the space that has the inline-size of the block that creates the block formatting context.  But as we reflow blocks that are descendants of the block establishing the BFC, we translate the space manager so that it knows how to translate from the coordinate space of the block we're reflowing into the coordinate space that it tracks floats in.

So the float manager needs to decide what its initial coordinate space is (i.e., whether it's relative to top-left or top-right), and then translate itself appropriately for
 descendant blocks.

 I also want to add the assertions that we don't switch a float manager between vertical and horizontal, since it doesn't make any sense at all to translate a f
loat manager between such states, and we should never have to, since a direction
 switch should create a new block formatting context (although it's not clear to me that we actually implement that, so the assertions might fire with vertical writing-mode enabled).
Here's a patch I started that adds a containerWidth field to the float manager, and then uses it to convert rects in AddFloat and GetFlowArea. Note that this is still a long way from sufficient to fix the issues here. (It does make a few of the currently-failing float-in-rtl reftests pass, though.)
Flags: needinfo?(jfkthame)
(In reply to David Baron [:dbaron] (UTC-8) from comment #32)
> Some other things I've noticed and patched locally are:
> https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> 6e75416cc3c8/dont-mix-up-bend

I'd also noticed this looked wrong, and patched locally.

> https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> 6e75416cc3c8/float-manager-wm-asserts

Makes sense; we should do this.

> direction
>  switch should create a new block formatting context (although it's not
> clear to me that we actually implement that, so the assertions might fire
> with vertical writing-mode enabled).

Right, we don't yet implement that. Bug 1134849 relates to this.
(In reply to David Baron [:dbaron] (UTC-8) from comment #35)

Thanks for this explanation, it's made things a lot clearer for me.

> So the float manager needs to decide what its initial coordinate space is
> (i.e., whether it's relative to top-left or top-right), and then translate
> itself appropriately for
>  descendant blocks.

So IIANM, instead of the current LogicalPoint mOffset, we should really be using a LogicalMargin and tracking the offsets of descendant blocks from both left and right of the initial coordinate space. Does that make sense?
Flags: needinfo?(smontagu)
Attachment #8576258 - Attachment is patch: true
Attachment #8576258 - Attachment mime type: message/rfc822 → text/plain
(In reply to Simon Montagu :smontagu from comment #38)
> So IIANM, instead of the current LogicalPoint mOffset, we should really be
> using a LogicalMargin and tracking the offsets of descendant blocks from
> both left and right of the initial coordinate space. Does that make sense?

I think a point (and I'm not even sure it needs to be a logical point) is probably sufficient; you just need to know the containing block width in order to do the translations.

At least, that's presuming that the space manager is storing its data in a sensible coordinate space, which I think means that it uses the block-start / block-end axis and the left-float / right-float axis.  (In other words, the space manager doesn't use logical directions in the inline axis.)

(Or is it possible, in vertical, for line-right and line-left to swap sides without the creation of a new block formatting context?  In that case, the float manager would need to be logicalized in an entirely different way.)
Er, I guess the last paragraph of my previous comment is true, so the
other advice is bad.  In order to do line-left and line-right correctly,
we'd need to use the caller's writing mode.

So the float manager probably needs to store stuff in coordinates that 
are block-start / block-end and then either: 
 (a) pick some absolute pair for the inline dimension (e.g., always
     left/right or top/bottom).
 (b) store its inline-dimension coordinates based on the line-left and
     line-right of the block that established the BFC, and translate 
     when needed.
I tend to think (a) is probably safer since we're likely to catch the 
bugs sooner, whereas with (b) the bugs would only show up in cases with
combinations of different vertical writing modes.
Very much so, see the penultimate paragraph in bug 1142318 comment 4
Depends on: 1142318
So even if bug 1142318 fixes this on trunk, on beta I'd rather go back to a known good state before bug 1062963. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e34eb80494ce is a try push for backing out patches from bugs 1062963, 1079139 and 1105137 to get to that state. There will probably need to be some changes to tests as well.
Attachment #8578912 - Flags: review?(jfkthame)
Attachment #8578912 - Flags: approval-mozilla-beta?
The other patches from bug 1062963 can stay, I think.

Requesting beta approval for this and the previous two attachments

Approval Request Comment
[Feature/regressing bug #]: The logical-coordinate float manager from bug 1062963 and followups
[User impact if declined]: Regressions in page layout in right-to-left pages with floats.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e34eb80494ce
[Risks and why]: Should be low risk, since it returns the tree to the state before bug 1062963 without introducing new code.
[String/UUID change made/needed]: none
Attachment #8578916 - Flags: review?(jfkthame)
Attachment #8578916 - Flags: approval-mozilla-beta?
(In reply to Simon Montagu :smontagu from comment #43)
> So even if bug 1142318 fixes this on trunk, on beta I'd rather go back to a
> known good state before bug 1062963.

OK, for beta I think that makes sense. It looks like we have a forward fix coming in bug 1142318, which we may be able to uplift to aurora as well, but for beta the backout is the lowest-risk option.
Attachment #8578912 - Flags: review?(jfkthame) → review+
Attachment #8578913 - Flags: review?(jfkthame)
Attachment #8578913 - Flags: review+
Attachment #8578913 - Flags: approval-mozilla-beta?
Comment on attachment 8578916 [details] [diff] [review]
Backout for beta patch 3: backout bug 1062963 patch 3

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

rs=me for the beta backouts; I agree this is the safest fix there.

Have you tried the new float-in-rtl reftests with a beta+backouts build? I think they should probably pass, which would be a useful additional check on the backouts.
Attachment #8578916 - Flags: review?(jfkthame) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #48)
> Have you tried the new float-in-rtl reftests with a beta+backouts build? I
> think they should probably pass, which would be a useful additional check on
> the backouts.

Great suggestion! They do pass :)
Comment on attachment 8578912 [details] [diff] [review]
Backout for beta patch 1: backout bug 1105137

After speaking with Simon, I'm approving the backout as this is our best option. Simon will handle the backout himself. Beta+
Attachment #8578912 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8578913 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8578916 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Jonathan Kew (:jfkthame) from comment #37)
> (In reply to David Baron [:dbaron] (UTC-8) from comment #32)
> > https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> > 6e75416cc3c8/float-manager-wm-asserts
> 
> Makes sense; we should do this.

Agreed, but I would go a step further and not let the float manager make any switch to the block direction, i.e. not mix vertical-lr and vertical-rl either.
Flags: in-testsuite+
Keywords: leave-open
Target Milestone: mozilla37 → ---
https://hg.mozilla.org/mozilla-central/rev/fb05b4b128da
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.