108187 - Indent wrong on RTL paragraph in Hebrew

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[nyh]

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.

Comment 1

[nyh]

Just checked, and the bug still exists on Mozilla 0.9.6

Comment 2

[Simon Montagu :smontagu]

Attachment: suggested patch

Comment 3

[Simon Montagu :smontagu]

Attachment: Alternative patch

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 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 6

[Simon Montagu :smontagu]

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>

Comment 7

[Simon Montagu :smontagu]

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?

Comment 8

[Simon Montagu :smontagu]

Attachment: New patch: move text indent further out

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> 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 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 11

[Simon Montagu :smontagu]

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...

Comment 12

[Simon Montagu :smontagu]

Attachment: attachment 60761 merged to tip

Comment 13

[Simon Montagu :smontagu]

Attachment: Oops! added formatting change suggested in comment 10

Comment 14

[Simon Montagu :smontagu]

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 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

r=dbaron

Comment 16

[Marc Attinasi]

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

sr=attinasi

Comment 17

[Simon Montagu :smontagu]

Fix checked in.

Comment 18

[(no longer active)]

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