1142318 - The text appears behind the image

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Simon Montagu :smontagu]

+++ This bug was initially created as a clone of Bug #1105137 +++

The text appears behind the image in page https://dl.dropboxusercontent.com/u/95157096/85f61cf7/ItJnkL8E9x.html.

Screenshot https://dl.dropboxusercontent.com/u/95157096/85f61cf7/Qxj3jZf7uG.png.

From bug 1114329 comment 32 and bug 1114329 comment 35, I realize that the fix for bug 1105137 was wrong. I suggested in bug 1114329 comment 38 an alternative approach to fix it.

Comment 1

[Simon Montagu :smontagu]

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

Comment 2

[Simon Montagu :smontagu]

Attachment: Backout the fix to bug 1105137

Comment 3

[Simon Montagu :smontagu]

Attachment: LineRelative writing mode

After discussions in bug 1114329 I now think the way to fix this is to make the float manager always use a writing mode with inline left-to-right (or top-to-bottom) direction. First I add a method to WritingModes to take a writing mode and return a writing mode forced to inline left-to-right.

Comment 4

[Simon Montagu :smontagu]

Attachment: Patch 3: make the float manager use the forced writing mode and a physical origin

A large percentage of this patch goes further back to before the patch for bug 1062963.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=622e47cd9128 (this is with an earlier version, I'll do another one after any changes following review and before checkin)

Meanwhile, this makes all the "float-in-rtl" testcases from bug 1114329 pass without any new reftest failures; also attachment 8564113 [details] and the other testcases and links in bug 1114329 and the testcases from bug 1105137 and bug 1113526.

Floats in vertical writing modes still get misplaced, but that probably depends on bug 1134849 and/or further work in that direction.

Comment 5

[Simon Montagu :smontagu]

By the way, this patch set makes nsFloatManager's mWritingMode no longer used, since all calculations are done in the caller's writing mode (or a variation of it as explained in comment 3). If you agree, I'll add a followup patch to make the float manager only store the writing mode in debug builds, like the logical geometry classes

Comment 6

[Jonathan Kew [:jfkthame]]

It's great to see a more reliable/consistent solution taking shape here. I've started to look at the patches, but will want a bit more "thinking time" before a proper review. One initial remark, though -- I'm not sure about the "LineRelativeWritingMode" term; that doesn't seem at all clear to me. But as yet I haven't come up with a term that I feel properly captures what's going on there. Bikeshed time! :) Were there other possibilities you considered along the way?

Comment 7

[Simon Montagu :smontagu]

"LineRelativeWritingMode" is based on the usage of line-relative (with line-left and line-right) in http://dev.w3.org/csswg/css-writing-modes. Why does it seem unclear?

Comment 8

[Jonathan Kew [:jfkthame]]

I'm OK with CSS-WM's discussion of line-relative directions; but I don't think of that as "another kind of writing mode" or a mapping of one writing mode to another. I suppose the idea here is that WritingMode::LineRelativeWritingMode() gives you a mode where inline-start and -end correspond to what would have been the original mode's line-left and -right.

I think I'd find it easier to understand accessors like LineLeft() and LineRight() on LogicalRect, for example, to map between the (flow-relative) logical values and line-relative; but I'm not clear yet whether that would a usable basis for what you're doing in this patch.

Comment 9

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #8)
> I think I'd find it easier to understand accessors like LineLeft() and
> LineRight() on LogicalRect, for example, to map between the (flow-relative)
> logical values and line-relative; but I'm not clear yet whether that would a
> usable basis for what you're doing in this patch.

I tried that before, but (at least the way I had it) it didn't work well. Now that I know better what I'm shooting for I would do it differently and I think I could make it work and it might indeed be clearer that way. I'll attach an alternative patch for your consideration.

Comment 10

[Jonathan Kew [:jfkthame]]

I'm also wondering whether the float-manager can store mI/mB in its writing mode, rather than mX/mY? We shouldn't be using the same float manager with both horizontal and vertical writing modes, I think (see also David's assertions, bug 1114329 comment 35). So could we avoid the need for the mI() and mB() accessors with their IsVertical() condition, if the float-manager never needs to deal with orthogonal modes?

Comment 11

[Jonathan Kew [:jfkthame]]

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

> Now that I know better what I'm shooting for I would do it differently and I
> think I could make it work and it might indeed be clearer that way. I'll
> attach an alternative patch for your consideration.

Thanks; I'll wait to look at that before trying to complete a review here.

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

> After discussions in bug 1114329 I now think the way to fix this is to make
> the float manager always use a writing mode with inline left-to-right (or
> top-to-bottom) direction. First I add a method to WritingModes to take a
> writing mode and return a writing mode forced to inline left-to-right.

Looking ahead to when we add support for text-orientation:sideways-left, remember that line-left in that case will become bottom rather than top. So will the float manager use an inline coordinate system that runs bottom-to-top (so it's still line-left to line-right), or will it stay with physical top-to-bottom, which will then be line-right-to-left?

(Not necessarily a question that needs to be settled right now, but maybe something to keep in mind so that we leave the door open for whatever extensions may be needed when we add that feature.)

Comment 12

[Simon Montagu :smontagu]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16b80fbc857f is a try push with a version adopting the suggestions from comment 8 and comment 10. On the whole I like it better than the previous version, and I'll request review tomorrow if all goes well.

(In reply to Jonathan Kew (:jfkthame) from comment #11)
> Looking ahead to when we add support for text-orientation:sideways-left,
> remember that line-left in that case will become bottom rather than top. So
> will the float manager use an inline coordinate system that runs
> bottom-to-top (so it's still line-left to line-right), or will it stay with
> physical top-to-bottom, which will then be line-right-to-left?

I did consider this already, and I believe that the former option is the way to go, so that a left float is always on the side of the line from which left-to-right text begins. Like other parts of support for sideways-left, this will require adding a containerHeight in various places as well as the containerWidth we already use.

Comment 13

[Simon Montagu :smontagu]

Attachment: Patch 2 alternative: add a LineLeft and LineRight accessor to LogicalRect

Comment 14

[Simon Montagu :smontagu]

Attachment: Patch 3 alternative: use LineLeft and LineRight to place floats

As I said above, I think this is better in almost every way. The use of a fake writing mode in FloatInfo.mRect is rather hacky, but I couldn't think of a better way to get the float coordinates and the translation into the same axes.

Comment 15

[Simon Montagu :smontagu]

Attachment: Mark reftests as passing

We need this as well to get rid of all the UNEXPECTED PASS messages.

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8579910 [details] [diff] [review]
Patch 2 alternative: add a LineLeft and LineRight accessor to LogicalRect

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

::: layout/generic/WritingModes.h
@@ +1480,5 @@
> +    if (aWritingMode.IsVertical()) {
> +      return IStart(); // sideways-left will require aContainerHeight
> +    } else {
> +      return aWritingMode.IsBidiLTR() ? IStart()
> +                                      : aContainerWidth - IStart() - ISize();

Can't we write this equivalently but more clearly (IMO) as

    aContainerWidth - IEnd();

?

@@ +1490,5 @@
> +    if (aWritingMode.IsVertical()) {
> +      return IEnd(); // sideways-left will require aContainerHeight
> +    } else {
> +      return aWritingMode.IsBidiLTR() ? IEnd()
> +                                      : aContainerWidth - IEnd() + ISize();

And this as

    aContainerWidth - IStart();

Comment 17

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #16)
> Can't we write this equivalently but more clearly (IMO) as
> 
>     aContainerWidth - IEnd();
> 
> ?

Yeah, that'll work.

> And this as
> 
>     aContainerWidth - IStart();

Ditto (and there are couple of places in the next patch that can be similarly simplified)

Comment 18

[Simon Montagu :smontagu]

(In reply to Simon Montagu :smontagu from comment #17)
> there are couple of places in the next patch that can be
> similarly simplified

Or not, that statement was probably too hasty

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 8579910 [details] [diff] [review]
Patch 2 alternative: add a LineLeft and LineRight accessor to LogicalRect

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

r=me, with the suggested simplification. Also, let's remove the commented-out ILeft()/IRight() methods in LogicalRect, which I think were envisaged as being these accessors, but were never actually correct (or used).

Comment 20

[Jonathan Kew [:jfkthame]]

Comment on attachment 8579916 [details] [diff] [review]
Patch 3 alternative: use LineLeft and LineRight to place floats

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

Yes, I think this is much better. A couple of further suggestions, which I've experimented with locally; I'll post a followup patch that implements these changes, for your consideration. It's intended to make no difference to behavior, only to make things clearer in the code.

::: layout/generic/nsBlockReflowContext.cpp
@@ +251,5 @@
>        aFrameRS.AvailableBSize() -= mBStartMargin.get() + aClearance;
>      }
>    }
>  
> +  nscoord tI=0, tB=0;

nit: spaces around '=' signs.

::: layout/generic/nsFloatManager.h
@@ +49,5 @@
> +            // convert back from LineLeft to IStart
> +            aWritingMode.IsVertical() || aWritingMode.IsBidiLTR()
> +              ? aLineLeft
> +              : aContainerWidth - aISize - aLineLeft,
> +            aBCoord, aISize, aBSize)

I'd suggest doing this conversion at the (only) call site, rather than "hiding" it in a constructor with a special interpretation of its first nscoord parameter, which is any other similar place would be an inline-coord in the given writing mode.

@@ +332,5 @@
> +    // NB! This rect uses a default writing mode internally, which may
> +    // not be the actual writing mode of the frame manager. Its inline
> +    // coordinates are in the line-relative axis of the frame manager
> +    // and its block coordinates are in the frame manager's real block
> +    // direction.

Instead of this "fake writing mode", I suggest we simply have individual fields here:

  nscoord mLineLeft, mBStart, mISize, mBSize

and adjust the code that uses the mRect fields to work with these instead. And the mWritingMode field can be dropped altogether. I think that'll be less confusing than using a LogicalRect that doesn't have a true writing mode anyway.

Comment 21

[Jonathan Kew [:jfkthame]]

Attachment: followup

This applies on top of your patches here; the result should be exactly equivalent, but I think it makes for less potential confusion in the code. See what you think.

Comment 22

[Jonathan Kew [:jfkthame]]

Comment on attachment 8579917 [details] [diff] [review]
Mark reftests as passing

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

Awesome :)

Comment 23

[Jonathan Kew [:jfkthame]]

Oh, and as a further followup, I think we can drop the mWritingMode field from nsFloatManager altogether, right? Or better, make it DebugOnly<>, and use it to assert writing-mode compatibility (modes are not orthogonal, or the stronger condition that GetBlockDir() should match?) with the writing modes passed by callers of AddFloat, GetFlowArea, etc.

Comment 24

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #23)
> Oh, and as a further followup, I think we can drop the mWritingMode field
> from nsFloatManager altogether, right? Or better, make it DebugOnly<>, and
> use it to assert writing-mode compatibility (modes are not orthogonal, or
> the stronger condition that GetBlockDir() should match?) with the writing
> modes passed by callers of AddFloat, GetFlowArea, etc.

I tried doing so, and the assertions fire freely. :( Filed bug 1145218 to follow up on this.

Comment 25

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #23)
> Oh, and as a further followup, I think we can drop the mWritingMode field
> from nsFloatManager altogether, right? Or better, make it DebugOnly<>, and
> use it to assert writing-mode compatibility (modes are not orthogonal, or
> the stronger condition that GetBlockDir() should match?) with the writing
> modes passed by callers of AddFloat, GetFlowArea, etc.

Yes, I said this already in comment 5

Comment 26

[Simon Montagu :smontagu]

Comment on attachment 8580056 [details] [diff] [review]
followup

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

I'll make the changes in WritingModes.h and nsBlockReflowContext.cpp before checkin, but I don't agree with splitting out the individual coordinates in FloatInfo. I had it that way last night in the version I pushed to try, but on looking over it again in the cold light of morning I decided it was the lesser of two evils to (ab)use our existing data structures rather than reinventing lots of wheels by doing manual calculations like mBStart + mBSize etc etc.

::: layout/generic/nsFloatManager.cpp
@@ +176,5 @@
>      if (fi.mLeftBEnd <= blockStart && fi.mRightBEnd <= blockStart) {
>        // There aren't any more floats that could intersect this band.
>        break;
>      }
> +    if (fi.mISize == 0 && fi.mBSize == 0) {

I discovered in the try push that this should be 

 if (fi.mISize <= 0 || fi.mBSize <= 0)

&& instead of || causes an UNEXPECTED PASS in reftests/floats/zero-height-float.html, and I don't know if that's a good thing or a bad thing (see bug 81710)

I think it was chiefly this unexpected change of behaviour that convinced me that it was better practice to use a LogicalRect in FloatInfo after all.

@@ +234,5 @@
>    nscoord blockSize = (blockEnd == nscoord_MAX) ?
>                         nscoord_MAX : (blockEnd - blockStart);
> +
> +  // convert back from LineLeft/Right to IStart
> +  nscoord inlineStart = aWM.IsVertical() || aWM.IsBidiLTR()

Yes, fair enough, I'll make this change locally too.

Comment 27

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #26)
> Comment on attachment 8580056 [details] [diff] [review]
> followup
> 
> Review of attachment 8580056 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll make the changes in WritingModes.h and nsBlockReflowContext.cpp before
> checkin, but I don't agree with splitting out the individual coordinates in
> FloatInfo. I had it that way last night in the version I pushed to try, but
> on looking over it again in the cold light of morning I decided it was the
> lesser of two evils to (ab)use our existing data structures rather than
> reinventing lots of wheels by doing manual calculations like mBStart +
> mBSize etc etc.

Hmm. OK.... though in that case, how about simply (ab)using an nsRect and its methods? The writing-mode field of the LogicalRect is serving no purpose here, so we might as well drop it and use the underlying nsRect directly.

> I discovered in the try push that this should be 
> 
>  if (fi.mISize <= 0 || fi.mBSize <= 0)
> 
> && instead of || causes an UNEXPECTED PASS in
> reftests/floats/zero-height-float.html, and I don't know if that's a good
> thing or a bad thing (see bug 81710)
> 
> I think it was chiefly this unexpected change of behaviour that convinced me
> that it was better practice to use a LogicalRect in FloatInfo after all.

Argh... I knew I ought to have checked what IsEmpty() really meant, rather than just making it up!

Comment 28

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #27)
> Hmm. OK.... though in that case, how about simply (ab)using an nsRect and
> its methods? The writing-mode field of the LogicalRect is serving no purpose
> here, so we might as well drop it and use the underlying nsRect directly.

I tried that already too. It's probably the cleanest solution code-wise, but it's a little confusing to read, with things like 
 nscoord thisBEnd = info.mRect.YMost();
which make it *look* as if we're mixing logical and physical coordinates. I suppose that's not the end of the world though, and I can still add a comment like I did in the last version explaining what's going on.

Comment 29

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #28)
> (In reply to Jonathan Kew (:jfkthame) from comment #27)
> > Hmm. OK.... though in that case, how about simply (ab)using an nsRect and
> > its methods? The writing-mode field of the LogicalRect is serving no purpose
> > here, so we might as well drop it and use the underlying nsRect directly.
> 
> I tried that already too. It's probably the cleanest solution code-wise, but
> it's a little confusing to read, with things like 
>  nscoord thisBEnd = info.mRect.YMost();
> which make it *look* as if we're mixing logical and physical coordinates. I
> suppose that's not the end of the world though, and I can still add a
> comment like I did in the last version explaining what's going on.

In addition to a full comment at the declaration, explaining how we're going to abuse the rect, it might be worth appending a little "// line-relative" comment to those ugly mixed places in the code.

Or how about going a step further: make the mRect member of FloatInfo private, and instead use a bunch of accessors to (in effect) rename its fields to line-relative names for the code that uses it? We wouldn't need to forward anything like all of nsRect's methods, after all; we only use a handful of them.

  struct FloatInfo {
  ...
    nscoord LineLeft() const { return mRect.x; }
    nscoord LineRight() const { return mRect.XMost(); }
    nscoord LineOver() const { return mRect.y; }
    nscoord LineUnder() const { return mRect.YMost(); }
    bool IsEmpty() const { return mRect.IsEmpty(); }
  ...
  private:
    nsRect mRect;
  }

That would neatly encapsulate all the "abuse" in one place inside the FloatInfo struct, and allow the rest of the floatmanager code to be written more cleanly.

It seems like we've batted the various idea back and forth enough, and they're all a bit messy one way or another; I'll leave the final choice to you. FWIW, right now I'm leaning towards this last option.

Comment 30

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb05b4b128da
https://hg.mozilla.org/integration/mozilla-inbound/rev/93d75fb698ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/0db9fff35382
https://hg.mozilla.org/integration/mozilla-inbound/rev/95ec024d2e6b

(In reply to Jonathan Kew (:jfkthame) from comment #29)
> Or how about going a step further: make the mRect member of FloatInfo
> private, and instead use a bunch of accessors to (in effect) rename its
> fields to line-relative names for the code that uses it?

I think I like this option best, and it's what I've checked in. (I backed off from it before because I thought it might be considered overkill, but it really isn't (especially since it allows us to pare down some temporaries from call sites)).

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Wrong bug number in the commit message:
https://hg.mozilla.org/mozilla-central/rev/fb05b4b128da
https://hg.mozilla.org/mozilla-central/rev/93d75fb698ed
https://hg.mozilla.org/mozilla-central/rev/0db9fff35382
https://hg.mozilla.org/mozilla-central/rev/95ec024d2e6b

Comment 32

[Simon Montagu :smontagu]

Comment on attachment 8578186 [details] [diff] [review]
Backout the fix to bug 1105137

Approval Request Comment
[Feature/regressing bug #]: Original regression in bug 1062963, earlier patch in bug 1105137 didn't fix all cases
[User impact if declined]: Content of websites misplaced and even disappearing outside the viewport
[Describe test coverage new/current, TreeHerder]: Baked on trunk since 2015-03-23, tested with all the reported regressing sites in bug 1105137 and other dependencies of bug 1062963
[Risks and why]: Some risk of other regressions in float-based layout (hence no request for beta upload)
[String/UUID change made/needed]: None

Can this request recover all four r+ and checked-in patches in this bug, or do I need to request separately for each of them?

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8579917 [details] [diff] [review]
Mark reftests as passing

[Triage Comment]
Tests + we have the whole beta cycle to fix potential regressions. Taking it.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9d500d926e21
https://hg.mozilla.org/releases/mozilla-aurora/rev/16337cb1002a

Comment 35

[Ryan VanderMeulen [:RyanVM]]

*sigh* wrong bug numbers burned me again...
https://hg.mozilla.org/releases/mozilla-aurora/rev/d9e8b56ea593