Last Comment Bug 74880 - Lose indentation of RTL blockquote (nested RTL lists are fixed)
: Lose indentation of RTL blockquote (nested RTL lists are fixed)
Status: RESOLVED FIXED
: css-moz, rtl
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: P1 major with 14 votes (vote)
: mozilla1.7alpha
Assigned To: David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
:
Mentors:
: 211928 260715 (view as bug list)
Depends on: 419167
Blocks: 137995 195909 386957 177274 198869 206089
  Show dependency treegraph
 
Reported: 2001-04-05 11:36 PDT by Maha Abou El-Rous
Modified: 2010-09-14 07:14 PDT (History)
39 users (show)
asa: blocking1.4-
jwalden+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Ordered lists file (305 bytes, text/html)
2001-04-05 11:37 PDT, Maha Abou El-Rous
no flags Details
Similar problem with blockquotes (1.47 KB, text/html; charset=iso-8859-8-i)
2001-05-09 04:07 PDT, Simon Montagu :smontagu
no flags Details
suggested patch (3.58 KB, patch)
2002-06-02 04:53 PDT, Lina Kemmel
no flags Details | Diff | Splinter Review
RTL & LTR ordered lists with margins, paddings and borders (2.22 KB, text/html)
2002-07-30 03:47 PDT, Lina Kemmel
no flags Details
Screenshot of the above testcase in my build (28.48 KB, image/gif)
2002-07-30 04:13 PDT, Lina Kemmel
no flags Details
screen shot of the problem (86.81 KB, image/gif)
2002-08-24 12:43 PDT, Shoshannah Forbes
no flags Details
Example file showing the bug with both ordered and unordered lists (1.39 KB, text/html)
2003-02-05 12:48 PST, Nir Soffer
no flags Details
Fix based on the proposal in comment #35, with slight code changes (5.44 KB, patch)
2003-02-17 10:19 PST, Lina Kemmel
smontagu: review-
dbaron: superreview-
Details | Diff | Splinter Review
Testcase from bug 211928 (1.14 KB, text/html)
2003-07-07 11:42 PDT, Mats Palmgren (vacation)
no flags Details
patch (59.52 KB, patch)
2004-01-19 16:25 PST, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
no flags Details | Diff | Splinter Review
very simple testcase for new properties (3.33 KB, text/html; charset=UTF-8)
2004-01-19 16:38 PST, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
no flags Details
patch (checked in 2004-02-03 22:10 -0800) (65.38 KB, patch)
2004-02-03 21:08 PST, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
more serialization tweaks (9.51 KB, patch)
2004-02-04 14:50 PST, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
no flags Details | Diff | Splinter Review
more serialization tweaks (checked in 2004-02-04 16:19 -0800) (9.47 KB, patch)
2004-02-04 14:56 PST, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
quirks.css changes (877 bytes, patch)
2004-02-14 12:12 PST, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
mozilla: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
patch for ComputedMargin (1.29 KB, patch)
2004-12-11 08:14 PST, Hideo Saito
no flags Details | Diff | Splinter Review
testcase for attachment 168492 (828 bytes, text/html)
2004-12-11 08:20 PST, Hideo Saito
no flags Details
patch for margins (3.91 KB, patch)
2004-12-14 08:23 PST, Hideo Saito
no flags Details | Diff | Splinter Review
testcase for attachment 168702 (1.57 KB, text/html)
2004-12-14 08:28 PST, Hideo Saito
no flags Details
-moz-border-start-* and -moz-border-end-* (73.88 KB, patch)
2006-06-27 06:25 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
-moz-border-*-start and -moz-border-*-end (76.23 KB, patch)
2007-07-03 20:14 PDT, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
no flags Details | Diff | Splinter Review
interdiff between the previous two patches (36.91 KB, patch)
2007-07-03 20:16 PDT, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
no flags Details | Diff | Splinter Review
-moz-border-*-start and -moz-border-*-end (81.00 KB, patch)
2007-07-03 22:13 PDT, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
no flags Details | Diff | Splinter Review
interdiff between attachment 227241 and attachment 270835 (62.15 KB, patch)
2007-07-03 22:16 PDT, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
smontagu: review+
Details | Diff | Splinter Review

Description Maha Abou El-Rous 2001-04-05 11:36:13 PDT
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.
Comment 1 Maha Abou El-Rous 2001-04-05 11:37:55 PDT
Created attachment 29835 [details]
Ordered lists file
Comment 2 Frank Tang 2001-04-11 12:28:36 PDT
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
Comment 3 Frank Tang 2001-05-07 23:43:23 PDT
erik resign. reassign all his bug to ftang for now.
Comment 4 Frank Tang 2001-05-08 00:41:15 PDT
mark as assign for now.
simon- do you have a fix?
Comment 5 Frank Tang 2001-05-08 01:30:10 PDT
decommit other bidi issue until we land IBMBIDI default on.
Comment 6 Simon Montagu :smontagu 2001-05-09 04:07:11 PDT
Created attachment 33662 [details]
Similar problem with blockquotes
Comment 7 Simon Montagu :smontagu 2001-05-09 04:38:18 PDT
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?
Comment 8 Andreas Becker 2001-05-14 09:35:59 PDT
Changing QA contact to mahar@eg.ibm.com.
Comment 9 Frank Tang 2001-05-14 13:36:15 PDT
pierre and heikki:

can we use "*[dir=rtl] ol" ?
Comment 10 Frank Tang 2001-05-14 13:36:44 PDT
reassign to softel.il.co
Comment 11 Heikki Toivonen (remove -bugzilla when emailing directly) 2001-05-14 14:37:42 PDT
Pierre is on sabattical, and style is not my area. Adding Marc Attinasi to Cc.
Marc, any opinions?
Comment 12 Marc Attinasi 2001-05-14 15:26:08 PDT
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).
Comment 13 Simon Montagu :smontagu 2001-05-14 23:39:44 PDT
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.
Comment 14 Simon Montagu :smontagu 2001-05-15 23:25:50 PDT
Hoping for more input on this. Meanwhile taking off 0.9.1
Comment 15 Frank Tang 2001-07-18 09:37:40 PDT
move comonenet to Bidi Hebrew & Arabic
Comment 16 Zach Lipton [:zach] 2001-08-25 18:12:09 PDT
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. 
Comment 17 Katsuhiko Momoi 2001-08-25 19:44:52 PDT
BIdi UI option issues -- QA to maha for now.
Comment 18 Lina Kemmel 2002-06-02 04:53:03 PDT
Created attachment 85955 [details] [diff] [review]
suggested patch

I remain style margins as is, since the discussion around their mirroring
doesn't seem to be finished..
Comment 19 Lina Kemmel 2002-06-02 05:00:51 PDT
I guess I should say "mirroring them" instead of "their mirroring"..
Comment 20 Simon Montagu :smontagu 2002-06-03 10:17:16 PDT
I am cc-ing dbaron to this bug and lkemmel to bug 131023, since you seem to be
patching some of the same code.
Comment 21 Lina Kemmel 2002-06-04 08:11:55 PDT
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 22 Lina Kemmel 2002-06-04 08:12:24 PDT
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 23 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2002-07-04 16:26:45 PDT
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.
Comment 24 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2002-07-04 16:27:24 PDT
(Sorry for the long delay in commenting on this, too.)
Comment 25 Lina Kemmel 2002-07-16 04:37:20 PDT
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?
Comment 26 Lina Kemmel 2002-07-30 03:47:01 PDT
Created attachment 93270 [details]
RTL & LTR ordered lists with margins, paddings and borders
Comment 27 Lina Kemmel 2002-07-30 04:13:23 PDT
Created attachment 93271 [details]
Screenshot of the above testcase in my build

Margins, paddings and borders are flipped in the RTL list. Is that what we'd
expect to see?
Comment 28 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2002-07-30 17:36:34 PDT
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.
Comment 29 Lina Kemmel 2002-07-31 05:23:48 PDT
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.
Comment 30 Lina Kemmel 2002-07-31 05:58:02 PDT
Lots of ideas and nothing worthwhile..
It probably makes sense to set both left / right to 'auto' (?)
Comment 31 Lina Kemmel 2002-08-01 04:19:55 PDT
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.)
Comment 32 Eyal Rozenberg 2002-08-02 12:22:41 PDT
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!
Comment 33 Shoshannah Forbes 2002-08-24 12:42:40 PDT
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
Comment 34 Shoshannah Forbes 2002-08-24 12:43:58 PDT
Created attachment 96562 [details]
screen shot of the problem
Comment 35 Lina Kemmel 2002-08-27 07:24:32 PDT
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.)
Comment 36 Shoshannah Forbes 2002-08-27 13:30:10 PDT
Should this workaround work if the owner of the site adds it to his css?
It seems so, but can anyone confirm?
Comment 37 Lina Kemmel 2002-08-28 06:22:33 PDT
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.
Comment 38 Tsahi Asher 2002-08-28 09:57:13 PDT
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.
Comment 39 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2002-09-08 12:05:20 PDT
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.
Comment 40 Nir Soffer 2003-02-05 12:48:08 PST
Created attachment 113615 [details]
Example file showing the bug with both ordered and unordered lists

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.
Comment 41 Lina Kemmel 2003-02-17 10:19:21 PST
Created attachment 114683 [details] [diff] [review]
Fix based on the proposal in comment #35, with slight code changes

While there is no final decision which alternative to go with, I'd like to
suggest a simple fix. Can anyone review it? Thanks!
Comment 42 Simon Montagu :smontagu 2003-03-21 12:07:47 PST
See also http://bugs.kde.org/show_bug.cgi?id=56219
Comment 43 Ahmad A. Abu-Taha 2003-03-24 06:35:47 PST
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.
Comment 44 Prognathous 2003-04-26 17:16:32 PDT
Could someone please review the patch?

Prog.
Comment 45 Christopher Hoess (gone) 2003-04-26 17:53:49 PDT
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?
Comment 46 Roland Mainz 2003-04-27 16:21:05 PDT
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 47 Simon Montagu :smontagu 2003-04-28 10:50:19 PDT
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.
Comment 48 Lina Kemmel 2003-04-28 11:15:10 PDT
smontagu: I feel terrible that I didn't notice your proposal before. Sorry. But 
I really didn't intend to be plagiarist.
Comment 49 Lina Kemmel 2003-04-29 05:41:05 PDT
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.
Comment 50 Tsahi Asher 2003-04-29 05:45:59 PDT
i just wonder how long "temporary" is.
Comment 51 Lina Kemmel 2003-04-29 06:31:35 PDT
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.
Comment 52 Hixie (not reading bugmail) 2003-04-29 07:55:19 PDT
This patch is quite likely to break several top100 sites. If it does so, we must
be ready to back it out.
Comment 53 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2003-06-09 11:34:47 PDT
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 54 Lina Kemmel 2003-06-11 03:11:37 PDT
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 55 Lina Kemmel 2003-06-11 03:57:28 PDT
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 56 Lina Kemmel 2003-06-11 04:10:45 PDT
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.
Comment 57 Lina Kemmel 2003-06-12 01:39:05 PDT
Please disregard comment 56. Right margin may only derive left margin in a CSS 
rule.
Comment 58 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2003-07-07 10:55:04 PDT
*** Bug 211928 has been marked as a duplicate of this bug. ***
Comment 59 Mats Palmgren (vacation) 2003-07-07 11:42:35 PDT
Created attachment 127185 [details]
Testcase from bug 211928
Comment 60 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-01-19 16:25:35 PST
Created attachment 139456 [details] [diff] [review]
patch
Comment 61 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-01-19 16:38:26 PST
Created attachment 139458 [details]
very simple testcase for new properties
Comment 62 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-01-19 16:49:42 PST
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.
Comment 63 Hixie (not reading bugmail) 2004-01-21 11:11:53 PST
Wow.
Comment 64 Tsahi Asher 2004-01-23 17:20:42 PST
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.
Comment 65 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-01-23 18:36:47 PST
The testcase is testing properties that the patch uses in our default stylesheet.
Comment 66 Boris Zbarsky [:bz] 2004-02-03 20:00:39 PST
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.
Comment 67 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-02-03 21:08:31 PST
Created attachment 140573 [details] [diff] [review]
patch (checked in 2004-02-03 22:10 -0800)

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 68 Boris Zbarsky [:bz] 2004-02-03 21:20:24 PST
Comment on attachment 140573 [details] [diff] [review]
patch (checked in 2004-02-03 22:10 -0800)

r+sr=bzbarsky
Comment 69 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-02-03 22:13:33 PST
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.
Comment 70 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-02-04 14:50:03 PST
Created attachment 140624 [details] [diff] [review]
more serialization tweaks

This fixes the warnings showing up in the JS console.
Comment 71 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-02-04 14:56:02 PST
Created attachment 140626 [details] [diff] [review]
more serialization tweaks (checked in 2004-02-04 16:19 -0800)
Comment 72 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-02-04 14:59:49 PST
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 73 Boris Zbarsky [:bz] 2004-02-04 16:05:25 PST
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.
Comment 74 Boris Zbarsky [:bz] 2004-02-14 12:05:50 PST
Do we want to change the "dd not in dl" quirk in quirks.css to use margin-end too?
Comment 75 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-02-14 12:12:30 PST
Created attachment 141415 [details] [diff] [review]
quirks.css changes
Comment 76 Boris Zbarsky [:bz] 2004-02-14 12:25:48 PST
Comment on attachment 141415 [details] [diff] [review]
quirks.css changes

sr=bzbarsky
Comment 77 fantasai 2004-02-14 14:13:04 PST
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 78 Mike Kaply [:mkaply] 2004-03-02 11:10:10 PST
Comment on attachment 141415 [details] [diff] [review]
quirks.css changes

fantasai gave r= in a comment
Comment 79 Nir Soffer 2004-03-05 05:52:20 PST
this is a duplicate of http://bugzilla.mozilla.org/show_bug.cgi?id=198869 - which is FIXED
Comment 80 Prognathous 2004-03-05 11:02:24 PST
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.
Comment 81 Tsahi Asher 2004-03-06 04:11:48 PST
do the patches checked in here fix the problem with blockquotes too?
Comment 82 Tsahi Asher 2004-06-12 00:31:27 PDT
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?
Comment 83 Simon Montagu :smontagu 2004-06-12 00:45:46 PDT
The borders in <blockquote type=cite> (attachment 33662 [details]) are still not fixed.
Comment 84 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2004-09-29 16:57:25 PDT
bug 262195, regression?
Comment 85 Hideo Saito 2004-12-11 08:14:08 PST
Created attachment 168492 [details] [diff] [review]
patch for ComputedMargin

I think that both left and right computed margins of pearent reflow state
should be subtracted fom the available width.
Comment 86 Hideo Saito 2004-12-11 08:20:41 PST
Created attachment 168493 [details]
testcase for attachment 168492 [details] [diff] [review]
Comment 87 Hideo Saito 2004-12-14 08:23:27 PST
Created attachment 168702 [details] [diff] [review]
patch for margins
Comment 88 Hideo Saito 2004-12-14 08:28:21 PST
Created attachment 168703 [details]
testcase for attachment 168702 [details] [diff] [review]
Comment 89 Behdad Esfahbod 2005-05-16 18:10:25 PDT
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.
Comment 90 Hixie (not reading bugmail) 2005-05-17 10:31:13 PDT
before/after mean above/below, start/end mean left/right (in english; other
scripts have different mappings of course).
Comment 91 Eli Friedman 2006-02-22 17:51:47 PST
This is still open for a simple html.css change for blockquotes?  Or is there something I'm missing?
Comment 92 Simon Montagu :smontagu 2006-02-22 22:21:17 PST
(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)
Comment 93 Simon Montagu :smontagu 2006-06-27 06:25:44 PDT
Created attachment 227241 [details] [diff] [review]
-moz-border-start-* and -moz-border-end-*

I'm sure this needs more work, but it works in attachment 33662 [details] and all the variations that I could think of.
Comment 94 Eyal Rozenberg 2006-06-27 08:24:33 PDT
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.
Comment 95 Uri Bernstein (Google) 2006-06-27 08:29:10 PDT
(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. 
Comment 96 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-04-25 16:43:33 PDT
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.
Comment 97 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-07-03 14:41:52 PDT
I'll have a go at updating the patch for the tests...
Comment 98 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-07-03 20:14:09 PDT
Created attachment 270820 [details] [diff] [review]
-moz-border-*-start and -moz-border-*-end

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...
Comment 99 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-07-03 20:16:32 PDT
Created attachment 270821 [details] [diff] [review]
interdiff between the previous two patches
Comment 100 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-07-03 20:20:36 PDT
(In reply to comment #98)
>  we can use the nsStyleSides copy constructor,

Er, the nsCSSRect copy constructor.
Comment 101 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-07-03 20:34:52 PDT
Actually, I think I'll finish it up now anyway...
Comment 102 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-07-03 22:13:05 PDT
Created attachment 270835 [details] [diff] [review]
-moz-border-*-start and -moz-border-*-end

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.
Comment 103 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-07-03 22:16:08 PDT
Created attachment 270836 [details] [diff] [review]
interdiff between attachment 227241 [details] [diff] [review] and attachment 270835 [details] [diff] [review]

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.
Comment 104 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-07-04 11:59:27 PDT
Checked in to trunk.  Thanks for the patch, Simon.

And this bug is finally fully fixed.
Comment 105 Steve England [:stevee] 2008-02-23 04:32:19 PST
Could this have caused bug 419167?
Comment 106 :Ehsan Akhgari 2008-04-07 14:00:58 PDT
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Comment 107 Phil Ringnalda (:philor, back in August) 2009-01-24 12:55:28 PST
*** Bug 260715 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.