From Bugzilla Helper: User-Agent: Mozilla/4.0 (compatible; MSIE 5.01; Windows 98) BuildID: 00000000 - Bidi build dated 20010404 When setting the Bidi option orientation to RTL, the nested ordered lists loose their proper indentation Reproducible: Always Steps to Reproduce: 1.Open file ol.htm 2.From the View Menu - Bidi Options, select Orientation to be RTL Actual Results: The indented ordered lists are completely right aligned loosing their proper indentation Expected Results: The list should remain indented same as when displayed LTR. Check IE5 for comparable results.

mark as moz0.9.1 we are focusing on landing IBMBIDI without break Latin and CJK on moz0.9. BIDI functional problem move to moz0.9.1

erik resign. reassign all his bug to ftang for now.

mark as assign for now. simon- do you have a fix?

decommit other bidi issue until we land IBMBIDI default on.

There is a similar problem with <blockquote type=cite> (see attachment). In both these cases there is "padding-left" defined in html.css, and this is the cause of the problem. So what is the solution? I don't consider that it would be correct to mirror padding (and also margins and borders) when dir=rtl, and I haven't found a way to define a CSS rule for the right-to-left case. ol[dir=rtl] only works on an ol tag with a specific dir attribute, not when the dir attribute is inherited. Is that correct behaviour? (I think it is, but the CSS spec at http://www.w3.org/TR/REC-CSS2/selector.html#q10 is not totally explicit.) Is there another way to do it? Would it be acceptable to have left AND right padding on indented lists? Lots of questions here :-) Frank, can you consult some style gurus?

Changing QA contact to mahar@eg.ibm.com.

pierre and heikki: can we use "*[dir=rtl] ol" ?

reassign to softel.il.co

Pierre is on sabattical, and style is not my area. Adding Marc Attinasi to Cc. Marc, any opinions?

You could use "*[dir=rtl] ol" but that would not help if there was an element in between the one with dir="rtl" and the OL, as in <div dir="rtl"> <OL> <LI>RTL item </OL> <div dir="ltr"> <OL> <LI>LTR item </OL> </div> </div> The better solution is to use the direction-independent css values that will be specified in css3. CC'ing Ian in case he has a brilliant idea (or needs to correct me).

Marc, are you saying that css3 will be like xsl, and use values like before/after/start/end under the control of a global writing-mode? Way cool if so.

Hoping for more input on this. Meanwhile taking off 0.9.1

move comonenet to Bidi Hebrew & Arabic

Mass-move all BiDi Hebrew and Arabic qa to me, zach@zachlipton.com. Thank you Gilad for your service to this component, and best of luck to you in the future. Sholom.

BIdi UI option issues -- QA to maha for now.

I remain style margins as is, since the discussion around their mirroring doesn't seem to be finished..

I guess I should say "mirroring them" instead of "their mirroring"..

I am cc-ing dbaron to this bug and lkemmel to bug 131023, since you seem to be patching some of the same code.

The attached patch will work in "typical" cases and may be considered as a temporary workaround only. The CSS2 Box model specifies 3 areas surrounding the content area: padding, border, and margin. Each of these has left, right, top, and bottom segments (while only left and right are really among Bidi interests). Im mozilla, each of the segments seems to be composed from a CSS value and another programmatically set value (let's call it "computed" value). Sorry about repeating a previously asked question, but should we proceed according to CSS3 or according to IE?.. I.e. should we mirror the composed left / right segments or only their "computed" components?

The attached patch will work in "typical" cases and may be considered as a temporary workaround only. The CSS2 Box model specifies 3 areas surrounding the content area: padding, border, and margin. Each of these has left, right, top, and bottom segments (while only left and right are really among Bidi interests). Im mozilla, each of the segments seems to be composed from a CSS value and another programmatically set value (let's call it "computed" value). Sorry about repeating a previously asked question, but should we proceed according to CSS3 or according to IE?.. I.e. should we mirror the composed left / right segments or only their "computed" components?

Comment on attachment 85955 [details] [diff] [review] suggested patch > #ifdef IBMBIDI >- nsRect bounds(aLine->mBounds); >- >- if (mRect.x) { >- const nsStyleVisibility* vis; >- GetStyleData(eStyleStruct_Visibility, (const nsStyleStruct*&)vis); >- >- if (NS_STYLE_DIRECTION_RTL == vis->mDirection) { >- bounds.x += mRect.x; >- } >- } >- PRBool successful = aLineLayout.HorizontalAlignFrames(bounds, allowJustify, >- aState.GetFlag(BRS_SHRINKWRAPWIDTH)); > // XXX: not only bidi: right alignment can be broken after RelativePositionFrames!!! > // XXXldb Is something here considering relatively positioned frames at > // other than their original positions? >-#else >+#endif There's no need to have |#ifdef|s around a comment. :-) >+#ifdef IBMBIDI // fix for bug #74880 >+ const nsStyleVisibility* vis; >+ aFrame->GetStyleData(eStyleStruct_Visibility, (const nsStyleStruct*&)vis); >+ if (NS_STYLE_DIRECTION_RTL == vis->mDirection) >+ // take off only the borderPadding, but not the style margin. >+ aResult.x -= borderPadding.left; >+#endif This is what does the reversing, right? I'm really don't like that idea. Furthermore, this isn't reversing which side we draw the border on, but only where we leave room for the border. So I don't like this one part of the patch, but the rest seems fine to me (and should also fix bug 131023.

(Sorry for the long delay in commenting on this, too.)

David: >This is what does the reversing, right? I'm really don't like that idea. >Furthermore, this isn't reversing which side we draw the border on, but only >where we leave room for the border. I agree the fix is not satisfactory. (However, for the attached testcases that use the regular default css, it does the work.) I see 2 ways to mirror left / right indents: 1) Set desirable indents on the style structs in advance. 2) Adjust block rectangle to achieve desirable indents later. By the way, that's what http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsBlockReflowState.cp p#220 (without bidi code) seems to do. Do you agree? If so, which one is preferable in your opinion?

Margins, paddings and borders are flipped in the RTL list. Is that what we'd expect to see?

My memory is that the opinion of the CSS working group is that the margin-{left,right,top,bottom} properties mean just that. I could double-check if you want, but I think reversing is wrong.

Perusing the CSS2 spec and different discussions convinced me that you're right.. What do you think about specifying in css.html equal values for both right and left paddings (or whatever would be nice to have reversed in the current element)? Then wouldn't 'left' be meaningful and 'right' ignored in ltr direction; 'right' meaningful and 'left' ignored - in rtl.

Lots of ideas and nothing worthwhile.. It probably makes sense to set both left / right to 'auto' (?)

According to http://www.w3.org/TR/REC-CSS2/visudet.html#q6, for block-level, non-replaced elements in normal flow, with over-constrained margins and width, one of the left / right margins will be ignored depending on direction (css refers only to 'margin', but most likely has in mind paddings as well). So it seems that the following example should work: <style> blockquote[type=cite] { display: block; padding-left: 5%; padding-right: 5%; margin-left: 0px; margin-right: 0px; border-left-width: 0px; border-right-width: 0px; width: 95%; } </style> (My understanding that width percentage here is relative to the *generated* box's containing block, i.e. the padding edge of the *current* element.)

Have not noted this being said explicitly, so: 1. Not only nested ordered lists, but non-nested and also unordered lists are rendered ltr instead of rtl. 2. This is not a bug, it's a mega-bug! Aaagh!

Marking all as I am getting it with my osx 2002082211 build. Any work being done here? I got complainst from a mozilla user, with an unorderled list as well. see: http://www.law.co.il/computer-law/main.htm

It still looks like specifying equal values for left and right indents in the default css is a simple and harmless workaround to this problem. Though it makes text be cut off from both sides, it almost invisible in the absence of too deep nesting. Try to edit your $(INST_DIR)\res\html.css file (where $(INST_DIR) is mozilla's installation directory) as follows: 1) in the section 'blockquote[type=cite]' add the line padding-right: 1em; 2) in the section 'ul, menu, dir' add the line padding-right: 40px; 3) in the section 'ol' add the line padding-right: 40px; (and similarly other sections if needed.)

Should this workaround work if the owner of the site adds it to his css? It seems so, but can anyone confirm?

http://www.w3.org/TR/REC-CSS2/cascade.html#cascade - "By default, rules in author style sheets have more weight than rules in user style sheets. Precedence is reversed, however, for "!important" rules. All rules user and author rules have more weight than rules in the UA's default style sheet." See also http://www.w3.org/TR/REC-CSS2/cascade.html#cascading-order.

IMHO sites shouldn't bother to do that. mozilla should simply work as expected, without workarounds, that may or may not be implemented by the web site.

I suspect that the changes caused to existing pages by the proposal in comment 31 would have unacceptable affects on some existing pages. I could be wrong, though. Two alternatives would be: * implement {margin,padding,border,border-*}-{start,end} as described in http://lists.w3.org/Archives/Public/www-style/2002Aug/0396.html http://lists.w3.org/Archives/Public/www-style/2002Sep/0049.html * implement a pseudo-class selector such as :-moz-direction() that would match based on the 'direction' property of the parent element (somewhat ugly, since CSS itself doesn't have that type of dependency, I don't think, but implementable for us) I'm more confident that the former would be sufficient, although it's a bit of a pain.

The bug effect both <ol> and <ul> type lists. The attached file looks fine on MS Explorer 5.5 and 6, but breaks on Mozilla 1.2.1.

While there is no final decision which alternative to go with, I'd like to suggest a simple fix. Can anyone review it? Thanks!

See also http://bugs.kde.org/show_bug.cgi?id=56219

Kindly be informed that Ahmad A. Abu-Taha (ahtaha@eg.ibm.com) from IBM Egypt is replacing Maha Abou El-Rous (mahar@eg.ibm.com) in monitoring and receiving notifications of Mozilla bugs regarding BiDi Arabic.

Could someone please review the patch? Prog.

Comment on attachment 114683 [details] [diff] [review] Fix based on the proposal in comment #35, with slight code changes Putting in a review request. smontagu, can you recommend a super-reviewer?

Christopher Hoess wrote: > (From update of attachment 114683 [details] [diff] [review]) > Putting in a review request. smontagu, can you recommend a super-reviewer? What about rbs ? :)

Comment on attachment 114683 [details] [diff] [review] Fix based on the proposal in comment #35, with slight code changes I would suggest dbaron for sr, but meanwhile I don't think that indenting from both sides is what we want to do (even if I was the first one to bring the idea up in this bug, I never really liked it). Personally I believe that the "right" fix is the first alternative mentioned in comment 39.

smontagu: I feel terrible that I didn't notice your proposal before. Sorry. But I really didn't intend to be plagiarist.

Comment on attachment 114683 [details] [diff] [review] Fix based on the proposal in comment #35, with slight code changes I'd like to remind that it provides a temporary fix - which, however, works around a number of layout problems. I also think that the 1st alternative from comment 39 is the neatest one.

i just wonder how long "temporary" is.

Comment on attachment 114683 [details] [diff] [review] Fix based on the proposal in comment #35, with slight code changes I need to correct myself: the fix is "temporary" only in its "css part"; all code changes are supposed to be "permanent" and are mandatory for example to block bug 195909.

This patch is quite likely to break several top100 sites. If it does so, we must be ready to back it out.

Comment on attachment 114683 [details] [diff] [review] Fix based on the proposal in comment #35, with slight code changes I think I've already said no to the css changes here, but marking sr- anyway. The other changes are probably something we want, but it would be useful to have more information over what they're supposed to fix before reviewing them.

Comment on attachment 114683 [details] [diff] [review] Fix based on the proposal in comment #35, with slight code changes Description of the changes: 1. In RTL list item, distance a bullet by its left (and not right) margin. 2. Set bullet's bidi levels. This is mostly important for inside bullet, but is also used for both types during selection. 3. Remove old changes that distanced content from a line box. Instead, use CSS- specified margins (at the moment not correctly implemented).

Comment on attachment 114683 [details] [diff] [review] Fix based on the proposal in comment #35, with slight code changes > 1. In RTL list item, distance a bullet by its left (and not right) margin. In the future, follow the "same source for various destinations" scheme as David proposed (comment 39), and use the right margin.

Comment on attachment 114683 [details] [diff] [review] Fix based on the proposal in comment #35, with slight code changes > 1. In RTL list item, distance a bullet by its left (and not right) margin. So it probabaly makes sense to back it out.

Please disregard comment 56. Right margin may only derive left margin in a CSS rule.

*** Bug 211928 has been marked as a duplicate of this bug. ***

This patch only does padding and margin, not border (yet), so I haven't changed blockquote[type=cite] yet. I want to see what the memory and performance implications of this patch are before going further.

Wow.

the test case in attachment 139458 [details] looks exactly the same in mozilla 1.5 and IE6, while IE has an excellent BiDi support. if i understand correctly, and you introduce new -moz- properties to address the problem, then your solution is not a solution. no one will use these properties just for mozilla users. mozilla should render even pure html, with no CSS at all, correctly.

The testcase is testing properties that the patch uses in our default stylesheet.

Comment on attachment 139456 [details] [diff] [review] patch >Index: content/base/src/nsRuleNode.cpp >+nsRuleNode::AdjustLogicalBoxProp(nsStyleContext* aContext, This function could use a nice comment either here or in the .h that explains what it's doing (especially what the args are). Perhaps the comment could include an example (eg of how "margin-start: 100px; margin-left: 20px" expands into the longhand props). >+ if (LTRlogical || RTLlogical) { >+ aInherited = PR_TRUE; Why is that necessary? It's not immediately clear to me, to be truthful.... Comment it, please? >Index: content/html/style/src/nsCSSParser.cpp >+PRBool CSSParserImpl::ParseDirectionalBoxProperty(nsresult& aErrorCode, Could we do a debug-only loop here over |subprops| and assert if there are either more or fewer than 3 subprops before we hit eCSSProperty_UNKNOWN? >Index: content/shared/public/nsCSSPropList.h >+CSS_PROP_MARGIN(margin-end-value, margin_end_value, X, Margin, mMarginEnd, eCSSType_Value, PR_TRUE) Is there a good reason not to have a useful method name here and for the other internal props? The other internal props have them.... The last looks pretty good, except for one issue -- round-tripping. nsCSSDeclaration::GetValue probably needs to be fixed to deal with the new shorthands.... nsCSSDeclaration::ToString needs to use the _value forms of the left/right padding/margin in the switch; otherwise TryFourSidesShorthand will probably break. In fact, all the margin/padding shorthand stuff here may need redoing to properly take into account the ltr and rtl sources. If you want, I can write some tests to test the roundtripping here; lemme know. It may be simplest to just hook up some alerts to the testcase you already posted.

I'll file a followup bug on serialization of rules to handle *-source when both *-left/right and *-start/end are in the rule (since that means order matters). I think I fixed the other round-tripping issues, though (two added changes to nsCSSDeclaration.cpp). I don't need a debug-only loop over subprops, since AppendValue has an assertion. I only need to assert that subprops[3] is UNKNOWN. I think I'd rather not have a useful method name for the internal props. I should probably change the others, but not now...

Comment on attachment 140573 [details] [diff] [review] patch (checked in 2004-02-03 22:10 -0800) r+sr=bzbarsky

Comment on attachment 140573 [details] [diff] [review] patch (checked in 2004-02-03 22:10 -0800) Checked in to trunk, 2004-02-03 22:10 -0800.

This fixes the warnings showing up in the JS console.

Comment on attachment 140626 [details] [diff] [review] more serialization tweaks (checked in 2004-02-04 16:19 -0800) Actually, I think I'd rather switch to aString consistently rather than aResult consistently (since it's an append rather than an assign).

Comment on attachment 140626 [details] [diff] [review] more serialization tweaks (checked in 2004-02-04 16:19 -0800) >Index: nsCSSDeclaration.cpp >+ NS_CASE_OUTPUT_PROPERTY_VALUE_AS(eCSSProperty_margin_left, eCSSProperty_margin_left_value, marginLeft) Shouldn't eCSSProperty_margin_left and eCSSProperty_margin_left_value be in the other order? Same for the other NS_CASE_OUTPUT_PROPERTY_VALUE_AS cases. r+sr=bzbarsky with that.

Do we want to change the "dd not in dl" quirk in quirks.css to use margin-end too?

Comment on attachment 141415 [details] [diff] [review] quirks.css changes sr=bzbarsky

If -moz-margin-start and -moz-margin-end do what I think they do, (and judging by a quick scan of the patch comments and html.css changes, I think they do), then r=fantasai

Comment on attachment 141415 [details] [diff] [review] quirks.css changes fantasai gave r= in a comment

this is a duplicate of http://bugzilla.mozilla.org/show_bug.cgi?id=198869 - which is FIXED

Nir, older bugs can't be duplicates of newer ones - it's the other way around. But that's besides the point. David, is there any reason why this bug is still open, now that the patch is checked in? Prog.

do the patches checked in here fix the problem with blockquotes too?

indentation is fixed for ol, ul, blockquote, dl, ul with nested ul, blockquote with nested blockquote, ul with nested dl, dl with nested ol. indeed, these are not all the possible combinations of these tags. is there any reason this bug is not marked fixed?

The borders in <blockquote type=cite> (attachment 33662 [details]) are still not fixed.

bug 262195, regression?

I think that both left and right computed margins of pearent reflow state should be subtracted fom the available width.

Humm, just for the record, isn't the proposed piece below: ul { -moz-margin-start: 40px; } equivalent to: ul:before { margin: 0; margin-right: 40px; } Most probably not, I'm not sure how :before and :after are handled for block elements. That cannot solve the border/padding problem anyway. I prefer -moz-margin-before/after to -moz-margin-start/end, but that's not really important.

before/after mean above/below, start/end mean left/right (in english; other scripts have different mappings of course).

This is still open for a simple html.css change for blockquotes? Or is there something I'm missing?

(In reply to comment #91) > This is still open for a simple html.css change for blockquotes? Or is there > something I'm missing? Blockquotes would also require C++ changes to borders, similar to the changes already made for margins and padding. (We may need those changes anyway to fix bug 328168)

I'm sure this needs more work, but it works in attachment 33662 [details] and all the variations that I could think of.

I still don't understand what constitutes a manifestation of this bug. What's wrong exactly with the rendering of the testcases? I don't see anything that's completely right-aligned and losing indentation.

(In reply to comment #94) > I still don't understand what constitutes a manifestation of this bug. What's > wrong exactly with the rendering of the testcases? in attachment 33662 [details] ("Similar problem with blockquotes") the blue borders in the RTL section are on the left, instead of on the right, and there is no indentation. The rest of this bug was fixed by dbaron's patches back in February 2004.

Sorry for not getting to reviewing this for so long. I've now added tests in layout/style/test/ that should test most of the things that I used to check for when adding new CSS properties. If you update the patch to make the appropriate additions to property_database.js and then make sure that no new test failures (red) are introduced, I should be able to review the patch much more quickly. See bug 279246 comment 41 for more details on how to do this. Note that this patch in particular might be a little tricky for updating the tests: you might need to change some existing properties to CSS_TYPE_SHORTHAND_AND_LONGHAND since they're now pseudo-shorthands. Having to add expected failures (which are noted in the script in the test files) for the cases where analogous properties already have failures is also acceptable, although it would be good to get bugs filed (blocking this bug and bug 377731) on fixing those issues.

I'll have a go at updating the patch for the tests...

So I merged this to the trunk and then tried to make it pass the tests. Just looking at it, I noticed that the subproperty list for 'border' was now insufficient, and fixing that required making changes to ParseBorder to avoid massive assertion spew. Then I got to work on fixing the tests -- some tweaks to the tests themselves, and a bunch of bug fixes to the patch. The tests pass with this patch. However, since the tests of computed values are disabled since the logical properties don't have computed values, the tests don't test the nsRuleNode changes, which have a bunch of bugs in them -- not supporting 'inherit' correctly, for example. The nsRuleNode changes also add more code than needed: we can use the nsStyleSides copy constructor, move AdjustLogicalBoxProps up to before the property computation, and avoid adding new methods for each of the types of adjusted properties. We also need a testcase to test the nsRuleNode code -- basically a custom JS mochitest that tests all of these properties (including the margin and padding ones) in various combinations, including 'inherit' and '-moz-initial' values, and including combinations with multiple properties specified to make sure they cascade correctly. So there's a good bit more to be done, and I've already spent more time than I'd intended on this...

(In reply to comment #98) > we can use the nsStyleSides copy constructor, Er, the nsCSSRect copy constructor.

Actually, I think I'll finish it up now anyway...

This one contains the test I mentioned and the fixes to nsRuleNode, reverts a bad change in the previous patch, and fixes a serious bug with border-left-width handling that was caught by my test.

Requesting review from Simon on the interdiff between his patch (merged to trunk, although it applied with patch -F8 with maximum fuzz of 5) and my patch.

Checked in to trunk. Thanks for the patch, Simon. And this bug is finally fully fixed.

Could this have caused bug 419167?

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