489517 - Setting direction to rtl doesn't enable Bidi processing

Status: RESOLVED FIXED

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

Description

[Simon Montagu :smontagu]

Attachment: testcase

This is a regression from bug 441367 caused by the change to "direction" in nsRuleNode::ComputeVisibilityData, specifically removing:

-    if (NS_STYLE_DIRECTION_RTL == visibility->mDirection)
-      mPresContext->SetBidiEnabled();

Adding a similar line in nsCSSFrameConstructor presumably prevented this causing a regression in static pages, but it doesn't help when changing direction dynamically.

Comment 1

[Simon Montagu :smontagu]

Attachment: Reference rendering

Comment 2

[Simon Montagu :smontagu]

Attachment: Patch

Putting back that line fixes the testcase -- not yet tested otherwise.

Comment 3

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

Can we move it from nsCSSFrameConstructor to nsFrame::DidSetStyleContext instead?  For the dynamic change case I'm not sure if we're guaranteed to get the visibility struct and trigger this code.

Comment 4

[Simon Montagu :smontagu]

Attachment: Move it to DidSetStyleContext

Doing it that way fixes the regression and doesn't appear to cause any new regressions in reftests or layout mochitests.

Since the code I'm moving references bug 115921, I added a couple of reftests for that as well as the testcase here.

Comment 5

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

Comment on attachment 374045 [details] [diff] [review]
Move it to DidSetStyleContext

r+sr=dbaron

It's probably better if the tests that show "No new line at end of file" in the diff have a newline at the end of the file.

Comment 6

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/c63c9e5b421c

Comment 7

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

Given that I marked this blocking, could you land it on branch too?

Comment 8

[Simon Montagu :smontagu]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5806c46219b5