Indent wrong on RTL paragraph in Hebrew

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
18 years ago
11 years ago

People

(Reporter: nyh, Assigned: mkaply)

Tracking

(Blocks 1 bug, {rtl})

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

18 years ago
I'm using Mozilla 0.9.4 on Redhat Linux 7.2.

The linked self-explanatory test-case shows how paragraph indents are done
correctly in either DIR=LTR or DIR=RTL mode, but when you have Hebrew text in
DIR=RTL mode (this is obviously the standard case for Hebrew), something goes
wrong and the indentation is put in the wrong (left) side.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 1

18 years ago
Just checked, and the bug still exists on Mozilla 0.9.6
Posted patch suggested patch (obsolete) — Splinter Review
Posted patch Alternative patch (obsolete) — Splinter Review
This is an alternative version of the patch which moves all the text-indent
code out of |ApplyLeftMargin| in order not to bloat the PerFrameData structure.
Comment on attachment 59990 [details] [diff] [review]
suggested patch

I think a cleaner way to follow the first approach would be to add a boolean
|mDidIndent| to something (I'm not sure what) and then check that boolean
rather than CanPlaceFloaterNow().  Or maybe CanPlaceFloaterNow is just wrong
and we need to rewrite it to be more appropriate -- this does seem like the bug
is really in that function. Even though its original intended use was quite
different, I think it's probably wrong there too.
Comment on attachment 60099 [details] [diff] [review]
Alternative patch

Bloating these structures isn't really an issue -- they're very transitory so
they shouldn't really contribute to bloat.

However, this does bring up a point, which is that it would probably make a lot
more sense if we applied the margins and text indent once when initializing the
line layout object rather than seeing if we need to before reflowing each
frame.	Perhaps this could even be done by moving some of this code out a few
functions into nsBlockFrame::ReflowInlineFrames, or something in nsLineLayout
called by ReflowInlineFrames (note the difference between ReflowInlineFrames
and ReflowInlineFrame).  Any such patch would probably have to be tested
carefully, but I'm thinking it might be the right way to go.  (I'm not really
sure though.)
The approach suggested in comment 5 sounds good to me, and I'll try implementing
it. 

<whinge>
The problem with modifying this kind of code is working out what breakage to
test for afterwards. I run the layout regression tests, but they seem to "cry
wolf" a lot.
</whinge>
More thoughts on comment 5: I tried moving the code for text-indent out to
nsLineLayout::BeginLineReflow, and couldn't find anything that broke as a
result. I am also seeing an average improvement of 2-3% on page load times.

But I don't think this will work with margins, since they can apply to inline as
well as block elements, so there seems to be no choice but to have every frame
check its own margins. Does this make sense?
Attachment #59990 - Attachment is obsolete: true
Attachment #60099 - Attachment is obsolete: true
> More thoughts on comment 5: I tried moving the code for text-indent out to
> nsLineLayout::BeginLineReflow, and couldn't find anything that broke as a
> result. I am also seeing an average improvement of 2-3% on page load times.

Sounds good.  (I'm a bit surprised it would be that much of an improvement,
though -- are you sure some of the change wsan't noise?)

Have you tried running the block and table regression tests?  See
http://mozilla.org/newlayout/regress.html  (They'll probably show you a bit of
noise where pages didn't really change, and you will likely have to compare a
bunch of pages side-by-side to see if there's any real change.)

> But I don't think this will work with margins, since they can apply to inline
> as well as block elements, so there seems to be no choice but to have every
> frame check its own margins. Does this make sense?

Yes.
Comment on attachment 60761 [details] [diff] [review]
New patch: move text indent further out

>+      nscoord width =
>+                     nsHTMLReflowState::GetContainingBlockContentWidth(mBlockReflowState->parentReflowState);

Looks like you introduced a tab here.
I've just rerun the pageload tests in a much quieter environment, and the
average improvement is more like 1.75%.

Running the regression tests now...
Attachment #60761 - Attachment is obsolete: true
I get 5 failures on the regression tests, but there are no visible changes when
I compare the testcase URLs.

I have removed the extra indentation (not actually a tab) which my editor
introduced while pasting, and merged to tip.
Comment on attachment 60864 [details] [diff] [review]
Oops! added formatting change suggested in comment 10

r=dbaron
Attachment #60864 - Flags: review+

Comment 16

18 years ago
Comment on attachment 60864 [details] [diff] [review]
Oops! added formatting change suggested in comment 10

sr=attinasi
Attachment #60864 - Flags: superreview+
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Updated

18 years ago
Blocks: 115713
Blocks: 137995
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl

Updated

11 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.