403328 - Absolute-positioned RTL table overflows to the right instead of to the left

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect, P2)

Description

[Uri Bernstein]

Attachment: testcase

In the example URL, the content of the form appears on the right, rather than inside the frame. This has to do with the overflowing of an absolute-positioned RTL table (where its containing block is too narrow to contain it).

The attached testcase should be symmetrical for the RTL (top) and LTR (bottom) cases, but it isn't: the RTL table overflows off-screen to the right.

This appears to be a regression from bug 328181.

The problem in the URL was reported by Nathaniel (נתנאל) on the mozilla.org.il forums.

Comment 1

[Uri Bernstein]

Attachment: testcase #2

This testcase shows that the bug also occurs when the table is replaced by two nested divs, the outer one having position:absolute, and the inner one having a specified width.
It does not occur when the width is specified directly on the absolute-positioned div.

Comment 2

[Uri Bernstein]

Attachment: patch

Use the direction of the "original" CB (i.e, the block that would have been the CB if this frame's position was "static"), instead of the direction of the actual CB, when determining how to position the frame.

I'm not sure this is true to the letter of the standard[1] (which I find to be a bit confusing), bit it's certainly the more sensible thing to do (and is also what Webkit trunk seems to be doing).

[1] http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-width

Comment 3

[Mike Schroepfer]

Setting to P3 since this is a regression. please adjust dbaron if necessary

Comment 4

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

Trunk rendering of attachment 288154 [details] agrees with WinIE.

I agree that your proposed change makes more sense for the first use of the 'direction' variable, but I'm not so sure about the other two (which are handling overconstrained margins).  I think you might want two different variables.

Are there any other browsers that match what you want other than Safari trunk?

What do browsers do on the overconstrained margins case?

Comment 5

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

Also, did you test that what they're doing is actually checking direction on the static-position-containing-block rather than direction on the element itself (as CSS 2.0 said)?

Comment 6

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

Attachment: unsent message to www-style

I need more testcases before I send this...

Comment 7

[Uri Bernstein]

Attachment: patch v2

(In reply to comment #4)
> I agree that your proposed change makes more sense for the first use of the
> 'direction' variable, but I'm not so sure about the other two (which are
> handling overconstrained margins).  I think you might want two different
> variables.

Actually, there are only two usages of "direction", not three. You are correct that it makes more sense to leave the second one unchanged (and this patch does this). However, I notice that Safari actually behaves as the original patch would have caused us to.

> 
> Are there any other browsers that match what you want other than Safari trunk?
> 

Not that I know of. Opera is brain-dead: it just assumes that the CB direction is LTR in all cases (even if both the actual and static-position CBs are RTL). I haven't tested IE6/IE7 yet.

> What do browsers do on the overconstrained margins case?

As said above, Safari (incorrectly) uses the direction of the "original" CB in this case too. I'll attach testcases and screenshots soon.

(In reply to comment #5)
> Also, did you test that what they're doing is actually checking direction on
> the static-position-containing-block rather than direction on the element
> itself (as CSS 2.0 said)?
> 

Yes, it appears that they are indeed checking the static-position-containing-block, not the element itself, as following testcases and screenshots will show.

Comment 8

[Uri Bernstein]

Attachment: testcase #3

This testcase is symmetrical, and shows the possible combinations when the direction of the "actual" (relative-positiones) containing block ("cb", lime) conflicts with that of the static-positioned-containing-block ("spcb", blue), and how this affects the positioning of the inner absolutely-positioned element ("abs", red).

Comment 9

[Uri Bernstein]

Attachment: testcase #4 (overconstrained margins)

similar, with overconstrained margins (actually, no margins are specified - the constrains are caused by specifying left, right, and width.

This testcase is affected by patch v1, but not by patch v2.

Comment 10

[Uri Bernstein]

Attachment: testcase #3 in Safari

This shows that Safari looks at the direction of the spcb, not of the absolute position itself.

Comment 11

[Uri Bernstein]

Attachment: testcase #4 in Safari

The previous comment should read "... not at the direction of the absolutely-positioned div itself". The same is true here.

Comment 12

[Uri Bernstein]

Attachment: Testcase #2 in IE7

(In reply to comment #4)
> Trunk rendering of attachment 288154 [details] agrees with WinIE.
> 

That's not what I'm seeing. In fact, as far as I can see, WinIE agrees with Safari on all the testcases (except that it messes up the center-alignment of the "spcb" - but that's a separate issue).

Comment 13

[Uri Bernstein]

(In reply to comment #6)
> Created an attachment (id=290943) [details]
> unsent message to www-style
> 
> I need more testcases before I send this...
> 

David, did you eventually send the letter? Are the testcases I provided sufficient? Is there anything else I can do to help here?

Comment 14

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

Which browsers is the current spec compatible with?  And what change to the spec would be needed to make it compatible with most browsers?  (And if that isn't the behavior you're trying to move us to with this patch, what change to the spec is needed to match this patch?)

Comment 15

[Uri Bernstein]

I tested on Safari 3, IE 7, and Opera 9 (and, of course Gecko trunk).
The current spec is only compatible with Gecko trunk.

There are two changes necessary to the spec in order to make it compatible with Safari 3, and an additional third one to make it compatible with IE7.

For the purpose of these changes, define the "static containing block" as "the rectangle that would have been the element's containing block if its 'position' property had been 'static' and 'float' had been 'none'".

1a. In the paragraph starting with "If none of the three is 'auto'", change the last sentence to: "If the values are over-constrained, ignore the value for 'left' (in case the 'direction' property of the *static* containing block is 'rtl') or 'right' (in case 'direction' is 'ltr') and solve for that value."

Additionally, in order to make the standard compatible with IE 7, *but not with Safari 3*:

1b. In the same paragraph, change the first sentence to end with: "...unless this would make them negative, in which case when direction of the *static* containing block is 'ltr' ('rtl'), set 'margin-left' ('margin-right') to zero and solve for 'margin-right' ('margin-left')."

2. In the list of cases following the paragraph starting with "Otherwise", change case 2 to :
# 'left' and 'right' are 'auto' and 'width' is not 'auto', then if the 'direction' property of the *static* containing block is 'ltr' set 'left' to the static position, otherwise set 'right' to the static position. Then solve for 'left' (if 'direction' is 'rtl') or 'right' (if 'direction' is 'ltr').

The behavior I'm trying to move us to with this (latest) patch requires change #2, but *not* changes #1a and #1b.

Alternatively, the original patch I submitted makes us compatible with IE 7, and requires all three changes. I prefer the behavior of the newer patch upon that of the original patch.

Comment 16

[Uri Bernstein]

Sorry, please disregard comment #15 for now - It's not fully correct. I'll try to post a correction soon.

Comment 17

[Uri Bernstein]

Attachment: comprehensive annotated testcase

This testcase demonstrates the four cases in which the spec references "the 'direction' property of the containing block", in the order in which they appear in the spec.

Note that this testcase triggers standards mode - which is significant for Safari (I didn't realize this before).

The following table describes how the first to red (absolutely-positioned) blocks are aligned in each of the four cases (A-D), for Mozilla trunk, IE, Safari in quirks mode, and Safari in standards mode, as well as the correct alignment according to the spec, and what I think should be the correct behaviour (the third and fourth red blocks are always aligned to the opposite side):

           A B C D   
Mozilla    L L L R
IE7        R R R R
Safari SM  L L L L
Safari QM  R R R R
CSS 2.1    L L L L
"correct:  R L L R

Comment 18

[Uri Bernstein]

So, based on the above testcase, here are my (hopefully correct, this time) replies to comment #14:

The current spec is compatible with Safari 3 in Standards Mode (but not with any other common browser).

There is no obvious change to the spec that would make it compatible with most browsers.

The behavior I'm trying to move us to with this patch requires the following changes to the spec:

Define the "static containing block" as "the
rectangle that would have been the element's containing block if its 'position'
property had been 'static' and 'float' had been 'none'".

In the following two paragraphs:

"If all three of 'left', 'width', and 'right' are 'auto': First set any 'auto' values for 'margin-left' and 'margin-right' to 0. Then, if the 'direction' property of the containing block is 'ltr' set 'left' to the static position and apply rule number three below; otherwise, set 'right' to the static position and apply rule number one below."

and

"2. If both 'left' and 'right' have the value 'auto', then if 'direction' of the containing block is 'ltr', set 'left' to the static position; else if 'direction' is 'rtl', set 'right' to the static position."

change "the 'direction' [property] of the containing block" to "the 'direction' [property] of the *static* containing block"

The paragraphs I'm suggesting to change are those that apply to cases A and D in the testcase above.

The logic behind this is that in these two cases, and only in these cases, positioning is done with regard to the "static position", and therefore the relevant direction to consider is that of the "static containing block".

Comment 19

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

Comment on attachment 291003 [details] [diff] [review]
patch v2

OK, r+sr=dbaron -- I agree this is the best behavior, and given that it does move us closer to compatibility with both IE7 and Safari quirks mode, I doubt we'll have compatibility problems with it.

I'm requesting approval for it now.  Please let me now whether you can check it in this week -- if not, I can land it for you.

Comment 20

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

It would also be good if you:
 * also tested replaced elements, since I think bullet (2) in 10.3.8 needs the same change
 * added reftests for as many of these cases as reasonable

Comment 21

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

(I presume that your patch also affects replaced elements.)

Comment 22

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

http://lists.w3.org/Archives/Public/www-style/2008Feb/0224.html

Comment 23

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

Comment on attachment 291003 [details] [diff] [review]
patch v2

Er, never mind about requesting approval -- this is blocking1.9+ so it doesn't currently need approval.

Comment 24

[Uri Bernstein]

Attachment: testcase for replaced elements

(In reply to comment #20)
> It would also be good if you:
>  * also tested replaced elements, since I think bullet (2) in 10.3.8 needs the
> same change

As far as I understand, bullet 2 in 10.3.8 corresponds to the "D" case (because replaced elements always have an intrinsic width), and therefore we already handle it correctly (i.e., /not/ the way the current spec specifies).
This testcase therefore renders correctly with the current code, and is not affected by my patch.

Comment 25

[Uri Bernstein]

Attachment: reftest

This is the reftest I'm going to check in with the patch. It's based on attachment 302176 [details]. I don't have much experience in creating reftests, so if there's something that shold be added/changed, please let me know and I'll do a followup.

Comment 26

[Uri Bernstein]

Fixed comments and checked in with a reftest:

Checking in mozilla/layout/generic/nsHTMLReflowState.cpp;
/cvsroot/mozilla/layout/generic/nsHTMLReflowState.cpp,v  <--  nsHTMLReflowState.cpp
new revision: 1.292; previous revision: 1.291
done
Checking in mozilla/layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.364; previous revision: 1.363
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/403328-1.html,v
done
Checking in mozilla/layout/reftests/bugs/403328-1.html;
/cvsroot/mozilla/layout/reftests/bugs/403328-1.html,v  <--  403328-1.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/403328-1-ref.html,v
done
Checking in mozilla/layout/reftests/bugs/403328-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/403328-1-ref.html,v  <--  403328-1-ref.html
initial revision: 1.1
done

Comment 27

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

I'd actually been working on a more involved reftest last night and this morning (since I'd thought you wouldn't be able to check it in).  I'll check that in in a bit, once the bugs it depends on (bug 419060 and bug 419072) are fixed.

Comment 28

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).