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

RESOLVED FIXED in mozilla1.7alpha

Status

()

defect
P1
major
RESOLVED FIXED
19 years ago
9 years ago

People

(Reporter: mrous, Assigned: dbaron)

Tracking

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

Trunk
mozilla1.7alpha
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
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
9.47 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
877 bytes, patch
mkaply
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
81.00 KB, patch
Details | Diff | Splinter Review
62.15 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
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.
Posted file Ordered lists file
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
erik resign. reassign all his bug to ftang for now.
Assignee: erik → ftang
mark as assign for now.
simon- do you have a fix?
Status: NEW → ASSIGNED
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.
QA Contact: andreasb → mahar
pierre and heikki:

can we use "*[dir=rtl] ol" ?
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?
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 → ---
move comonenet to Bidi Hebrew & Arabic
Component: Internationalization → 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. 
QA Contact: mahar → zach
BIdi UI option issues -- QA to maha for now.
QA Contact: zach → mahar
Blocks: 115713
Status: NEW → ASSIGNED
No longer blocks: 115713
Blocks: 137995
Posted patch suggested patch (obsolete) — Splinter Review
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
OS: Windows 98 → All
Hardware: PC → All
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.
Blocks: 177274
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!
Attachment #85955 - Attachment is obsolete: true
Blocks: 195909
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.
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)
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-
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.
Flags: blocking1.4? → blocking1.4-
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 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. ***
Posted patch patch (obsolete) — Splinter Review
Attachment #114683 - Attachment is obsolete: true
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
Attachment #139456 - Flags: superreview?(bz-vacation)
Attachment #139456 - Flags: review?(bz-vacation)
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...
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)
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)
Posted patch more serialization tweaks (obsolete) — Splinter Review
This fixes the warnings showing up in the JS console.
Attachment #140624 - Flags: superreview?(bzbarsky)
Attachment #140624 - Flags: review?(bzbarsky)
Attachment #140624 - Flags: superreview?(bzbarsky)
Attachment #140624 - Flags: review?(bzbarsky)
Attachment #140626 - Flags: superreview?(bzbarsky)
Attachment #140626 - Flags: review?(bzbarsky)
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+
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?
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+
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
Attachment #141415 - Flags: review?(fantasai) → review+
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.
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)
Posted patch patch for ComputedMargin (obsolete) — Splinter Review
I think that both left and right computed margins of pearent reflow state
should be subtracted fom the available width.
Posted file testcase for attachment 168492 (obsolete) —
Posted patch patch for margins (obsolete) — Splinter Review
Attachment #168492 - Attachment is obsolete: true
Posted file testcase for attachment 168702 (obsolete) —
Attachment #168493 - Attachment is obsolete: true
Attachment #168702 - Attachment is obsolete: true
Attachment #168703 - Attachment is obsolete: true
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.
Attachment #227241 - Flags: review?(dbaron)
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.
Attachment #33662 - Attachment mime type: text/html → text/html; charset=iso-8859-8-i
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...
Attachment #227241 - Attachment is obsolete: true
Attachment #227241 - Flags: review?(dbaron)
Attachment #270820 - Flags: superreview?(dbaron)
Attachment #270820 - Flags: review?(dbaron)
(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.
Attachment #270820 - Attachment is obsolete: true
Attachment #270821 - Attachment is obsolete: true
Attachment #270820 - Flags: superreview?(dbaron)
Attachment #270820 - Flags: review?(dbaron)
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)
Attachment #270836 - Flags: review?(smontagu) → review+
Checked in to trunk.  Thanks for the patch, Simon.

And this bug is finally fully fixed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 386957
Flags: in-testsuite+
Depends on: 419167
Could this have caused bug 419167?
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: mahar → layout.fonts-and-text
Duplicate of this bug: 260715
Keywords: css-moz
You need to log in before you can comment on or make changes to this bug.