3.25 KB, text/plain
2.75 KB, patch
|Details | Diff | Splinter Review|
3.21 KB, text/html
2.59 KB, text/html
5.25 KB, patch
|Details | Diff | Splinter Review|
Created attachment 288146 [details] 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.
Created attachment 288154 [details] 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.
Created attachment 288207 [details] [diff] [review] 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 (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).  http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-width
Setting to P3 since this is a regression. please adjust dbaron if necessary
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?
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)?
Created attachment 290943 [details] unsent message to www-style I need more testcases before I send this...
Created attachment 291003 [details] [diff] [review] 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.
Created attachment 291004 [details] 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).
Created attachment 291005 [details] 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.
Created attachment 291006 [details] testcase #3 in Safari This shows that Safari looks at the direction of the spcb, not of the absolute position itself.
Created attachment 291007 [details] testcase #4 in Safari The previous comment should read "... not at the direction of the absolutely-positioned div itself". The same is true here.
Created attachment 291054 [details] 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).
(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?
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?)
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.
Sorry, please disregard comment #15 for now - It's not fully correct. I'll try to post a correction soon.
Created attachment 302176 [details] 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
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 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.
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
(I presume that your patch also affects replaced elements.)
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.
Created attachment 304955 [details] 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.
Created attachment 304970 [details] [diff] [review] 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.
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
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.
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).