443629 - Changing bidi options no longer triggers reflow

Status: RESOLVED FIXED

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

Description

[Simon Montagu :smontagu]

Steps to reproduce (copied from bug 80352 comment 45. I'll add a better testcase later):

Go to a logical Bidi page, e.g. http://unicode.org/iuc/iuc10/x-ar.html
Open a new tab and use about:config to change bidi.texttype to 3
Return to the previous tab.

Expected results: the numbers within the Arabic text should be reversed:
 21-01 and 7991 instead of 10-12 and 1997.
Actual results: the numbers remain the same unless the page is reloaded.

Go to a visual Bidi page, e.g. http://www.cs.bgu.ac.il/~elad/noa_v.html
Open a new tab and use about:config to change bidi.texttype to 2
Return to the previous tab.

Expected results: all the Hebrew text should be reversed:
 העונ instead of נועה
Actual results: the text remains the same unless the page is reloaded.

Comment 1

[(no longer active)]

Since this blocks bug 441782, this should be targeted for 1.9.0.x if the other one is going to make it on a dot release.

Comment 2

[(no longer active)]

Are these two changes equivalent to the nsPresContext::ClearStyleDataAndReflow() function that was orirignally used when you wrote your patch to bug 80352 (attachment 202512 [details] [diff] [review])?

<http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/base&command=DIFF&root=/cvsroot&file=nsPresContext.cpp&rev1=3.341&rev2=3.342#6>

<http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/base&command=DIFF&root=/cvsroot&file=nsPresContext.cpp&rev1=3.343&rev2=3.344#10>

Comment 3

[(no longer active)]

The fix for this bug is included in the patch for bug 441782.

Comment 4

[Simon Montagu :smontagu]

FWIW, this regressed between 2007-07-25-04 and 2007-07-26-04, presumably by the changes to ClearStyleDataAndReflow in bug 386640.

Comment 5

[(no longer active)]

Hmm, I'm not sure I understand which part of the patch on bug 386640 has caused this regression.  I'm CCing Boris and David to see if they have any ideas.

Comment 6

[Simon Montagu :smontagu]

I think it's basically the same regression as bug 394057.

Comment 7

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

The mPrefChangePendingNeedsReflow = PR_TRUE in the patch in bug 441782 agrees with comment 6; the change that caused the regression was the assumption that things that did StyleChangedReflow were doing something actually changed style data in a way that would trigger reflow.  I fixed that assumption in bug 394057, but it seems I missed some cases (these bugs).

Comment 8

[(no longer active)]

OK then.  Marking this as a regression of bug 386640.  We can mark this as fixed once bug 441782 lands.  In the mean time I'm going to write a unit test for this.

Comment 9

[Samuel Sidler (old account; do not CC)]

Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release), but we'll look at a reviewed and baked patch (in the other bug).

Comment 10

[(no longer active)]

Fixed on mozilla-central by bug 441782.  roc: how can I go about testing this in an automated test?

Comment 11

[Simon Montagu :smontagu]

Does this need a separate testcase? IIRC if this regressed layout/base/tests/test_bug441782.html would fail.

Comment 12

[(no longer active)]

Yes, I think you're right.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

If this bug is covered in another test, then this is in‑testsuite+ :)