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.

Just checked, and the bug still exists on Mozilla 0.9.6

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?

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

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

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

Fix checked in.

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