Lose indentation of RTL blockquote (nested RTL lists are fixed)

RESOLVED FIXED in mozilla1.7alpha

Status

()

Core
Layout: Text
P1
major
RESOLVED FIXED
17 years ago
7 years ago

People

(Reporter: Maha Abou El-Rous, Assigned: dbaron)

Tracking

(Blocks: 3 bugs, {css-moz, rtl})

Trunk
mozilla1.7alpha
css-moz, rtl
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.4 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments, 11 obsolete attachments)

305 bytes, text/html
Details
1.47 KB, text/html; charset=iso-8859-8-i
Details
2.22 KB, text/html
Details
28.48 KB, image/gif
Details
86.81 KB, image/gif
Details
1.39 KB, text/html
Details
1.14 KB, text/html
Details
3.33 KB, text/html; charset=UTF-8
Details
65.38 KB, patch
Details | Diff | Splinter Review
9.47 KB, patch
Details | Diff | Splinter Review
877 bytes, patch
mkaply
: review+
Details | Diff | Splinter Review
81.00 KB, patch
Details | Diff | Splinter Review
62.15 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
Created attachment 29835 [details]
Ordered lists file

Comment 2

16 years ago
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
Target Milestone: --- → mozilla0.9.1

Comment 3

16 years ago
erik resign. reassign all his bug to ftang for now.
Assignee: erik → ftang

Comment 4

16 years ago
mark as assign for now.
simon- do you have a fix?
Status: NEW → ASSIGNED

Comment 5

16 years ago
decommit other bidi issue until we land IBMBIDI default on.
Created attachment 33662 [details]
Similar problem with blockquotes
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

16 years ago
Changing QA contact to mahar@eg.ibm.com.
QA Contact: andreasb → mahar

Comment 9

16 years ago
pierre and heikki:

can we use "*[dir=rtl] ol" ?

Comment 10

16 years ago
reassign to softel.il.co
Assignee: ftang → simon
Status: ASSIGNED → NEW
Pierre is on sabattical, and style is not my area. Adding Marc Attinasi to Cc.
Marc, any opinions?

Comment 12

16 years ago
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
Target Milestone: mozilla0.9.1 → ---

Comment 15

16 years ago
move comonenet to Bidi Hebrew & Arabic
Component: Internationalization → BiDi Hebrew & Arabic

Comment 16

16 years ago
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. 
QA Contact: mahar → zach

Comment 17

16 years ago
BIdi UI option issues -- QA to maha for now.
QA Contact: zach → mahar

Updated

16 years ago
Blocks: 115713

Updated

16 years ago
Status: NEW → ASSIGNED

Updated

15 years ago
No longer blocks: 115713

Updated

15 years ago
Blocks: 137995

Comment 18

15 years ago
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

15 years ago
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.

Comment 21

15 years ago
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

15 years ago
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?
(Assignee)

Comment 23

15 years ago
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.
(Assignee)

Comment 24

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

Comment 25

15 years ago
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

15 years ago
Created attachment 93270 [details]
RTL & LTR ordered lists with margins, paddings and borders

Comment 27

15 years ago
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?
(Assignee)

Comment 28

15 years ago
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

15 years ago
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

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

Comment 31

15 years ago
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

15 years ago
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

15 years ago
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
OS: Windows 98 → All
Hardware: PC → All

Comment 34

15 years ago
Created attachment 96562 [details]
screen shot of the problem

Comment 35

15 years ago
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

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

Comment 37

15 years ago
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

15 years ago
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.
(Assignee)

Comment 39

15 years ago
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.

Updated

15 years ago
Blocks: 177274

Comment 40

15 years ago
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

15 years ago
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!
Attachment #85955 - Attachment is obsolete: true

Updated

15 years ago
Blocks: 195909
See also http://bugs.kde.org/show_bug.cgi?id=56219

Comment 43

15 years ago
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

14 years ago
Could someone please review the patch?

Prog.
Flags: blocking1.4?
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?
Attachment #114683 - Flags: review?(smontagu)

Comment 46

14 years ago
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.
Attachment #114683 - Flags: superreview?(dbaron)
Attachment #114683 - Flags: review?(smontagu)
Attachment #114683 - Flags: review-

Comment 48

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

Comment 49

14 years ago
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

14 years ago
i just wonder how long "temporary" is.

Comment 51

14 years ago
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.

Updated

14 years ago
Flags: blocking1.4? → blocking1.4-
(Assignee)

Updated

14 years ago
Blocks: 198869, 206089
(Assignee)

Comment 53

14 years ago
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.
Attachment #114683 - Flags: superreview?(dbaron) → superreview-

Comment 54

14 years ago
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

14 years ago
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

14 years ago
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

14 years ago
Please disregard comment 56. Right margin may only derive left margin in a CSS 
rule.
(Assignee)

Comment 58

14 years ago
*** Bug 211928 has been marked as a duplicate of this bug. ***
Created attachment 127185 [details]
Testcase from bug 211928
(Assignee)

Comment 60

14 years ago
Created attachment 139456 [details] [diff] [review]
patch
Attachment #114683 - Attachment is obsolete: true
(Assignee)

Comment 61

14 years ago
Created attachment 139458 [details]
very simple testcase for new properties
(Assignee)

Comment 62

14 years ago
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.
Assignee: smontagu → dbaron
Status: ASSIGNED → NEW
Priority: -- → P1
Whiteboard: [patch]
Target Milestone: --- → mozilla1.7alpha
(Assignee)

Updated

14 years ago
Attachment #139456 - Flags: superreview?(bz-vacation)
Attachment #139456 - Flags: review?(bz-vacation)
Wow.

Comment 64

14 years ago
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.
(Assignee)

Comment 65

14 years ago
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.
(Assignee)

Comment 67

14 years ago
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...
(Assignee)

Updated

14 years ago
Attachment #139456 - Attachment is obsolete: true
Comment on attachment 140573 [details] [diff] [review]
patch (checked in 2004-02-03 22:10 -0800)

r+sr=bzbarsky
Attachment #140573 - Flags: superreview+
Attachment #140573 - Flags: review+
Attachment #139456 - Flags: superreview?(bzbarsky)
Attachment #139456 - Flags: review?(bzbarsky)
(Assignee)

Comment 69

14 years ago
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.
Attachment #140573 - Attachment description: patch → patch (checked in 2004-02-03 22:10 -0800)
(Assignee)

Comment 70

14 years ago
Created attachment 140624 [details] [diff] [review]
more serialization tweaks

This fixes the warnings showing up in the JS console.
(Assignee)

Updated

14 years ago
Attachment #140624 - Flags: superreview?(bzbarsky)
Attachment #140624 - Flags: review?(bzbarsky)
(Assignee)

Updated

14 years ago
Attachment #140624 - Flags: superreview?(bzbarsky)
Attachment #140624 - Flags: review?(bzbarsky)
(Assignee)

Comment 71

14 years ago
Created attachment 140626 [details] [diff] [review]
more serialization tweaks (checked in 2004-02-04 16:19 -0800)
Attachment #140624 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #140626 - Flags: superreview?(bzbarsky)
Attachment #140626 - Flags: review?(bzbarsky)
(Assignee)

Comment 72

14 years ago
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.
Attachment #140626 - Flags: superreview?(bzbarsky)
Attachment #140626 - Flags: superreview+
Attachment #140626 - Flags: review?(bzbarsky)
Attachment #140626 - Flags: review+
(Assignee)

Updated

14 years ago
Attachment #140626 - Attachment description: more serialization tweaks → more serialization tweaks (checked in 2004-02-04 16:19 -0800)
Do we want to change the "dd not in dl" quirk in quirks.css to use margin-end too?
(Assignee)

Comment 75

14 years ago
Created attachment 141415 [details] [diff] [review]
quirks.css changes
(Assignee)

Updated

14 years ago
Attachment #141415 - Flags: superreview?(bzbarsky)
Attachment #141415 - Flags: review?(fantasai)
Comment on attachment 141415 [details] [diff] [review]
quirks.css changes

sr=bzbarsky
Attachment #141415 - Flags: superreview?(bzbarsky) → superreview+

Comment 77

14 years ago
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

14 years ago
Comment on attachment 141415 [details] [diff] [review]
quirks.css changes

fantasai gave r= in a comment
Attachment #141415 - Flags: review?(fantasai) → review+

Comment 79

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

Comment 80

14 years ago
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

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

Comment 82

13 years ago
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.
Summary: Bidi: Lose indentation of nested ordered lists on RTL orientation → Lose indentation of RTL blcokquote (nested RTL lists are fixed)
Whiteboard: [patch]
Summary: Lose indentation of RTL blcokquote (nested RTL lists are fixed) → Lose indentation of RTL blockquote (nested RTL lists are fixed)
bug 262195, regression?

Comment 85

13 years ago
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

13 years ago
Created attachment 168493 [details]
testcase for attachment 168492 [details] [diff] [review]

Comment 87

13 years ago
Created attachment 168702 [details] [diff] [review]
patch for margins
Attachment #168492 - Attachment is obsolete: true

Comment 88

13 years ago
Created attachment 168703 [details]
testcase for attachment 168702 [details] [diff] [review]

Updated

13 years ago
Attachment #168493 - Attachment is obsolete: true

Updated

13 years ago
Attachment #168702 - Attachment is obsolete: true

Updated

13 years ago
Attachment #168703 - Attachment is obsolete: true

Comment 89

12 years ago
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).

Comment 91

12 years ago
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)
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.
Attachment #227241 - Flags: review?(dbaron)

Comment 94

11 years ago
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. 
(Assignee)

Comment 96

10 years ago
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.

Updated

10 years ago
Attachment #33662 - Attachment mime type: text/html → text/html; charset=iso-8859-8-i
(Assignee)

Comment 97

10 years ago
I'll have a go at updating the patch for the tests...
(Assignee)

Comment 98

10 years ago
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...
Attachment #227241 - Attachment is obsolete: true
Attachment #227241 - Flags: review?(dbaron)
Attachment #270820 - Flags: superreview?(dbaron)
Attachment #270820 - Flags: review?(dbaron)
(Assignee)

Comment 99

10 years ago
Created attachment 270821 [details] [diff] [review]
interdiff between the previous two patches
(Assignee)

Comment 100

10 years ago
(In reply to comment #98)
>  we can use the nsStyleSides copy constructor,

Er, the nsCSSRect copy constructor.
(Assignee)

Comment 101

10 years ago
Actually, I think I'll finish it up now anyway...
(Assignee)

Comment 102

10 years ago
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.
Attachment #270820 - Attachment is obsolete: true
Attachment #270821 - Attachment is obsolete: true
Attachment #270820 - Flags: superreview?(dbaron)
Attachment #270820 - Flags: review?(dbaron)
(Assignee)

Comment 103

10 years ago
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.
Attachment #270836 - Flags: review?(smontagu)

Updated

10 years ago
Attachment #270836 - Flags: review?(smontagu) → review+
(Assignee)

Comment 104

10 years ago
Checked in to trunk.  Thanks for the patch, Simon.

And this bug is finally fully fixed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Blocks: 386957
Flags: in-testsuite+

Updated

10 years ago
Depends on: 419167
Could this have caused bug 419167?
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl

Updated

9 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: mahar → layout.fonts-and-text
Duplicate of this bug: 260715

Updated

7 years ago
Keywords: css-moz
You need to log in before you can comment on or make changes to this bug.