Changing bidi options no longer triggers reflow

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Layout: Text
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: smontagu, Assigned: Away for a while)

Tracking

({fixed1.9.1, regression})

Trunk
mozilla1.9.2a1
fixed1.9.1, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fix in bug 441782)

(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
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.
Flags: wanted1.9.0.x?
(Assignee)

Comment 2

10 years ago
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>
(Assignee)

Comment 3

10 years ago
The fix for this bug is included in the patch for bug 441782.
Assignee: nobody → ehsan.akhgari
No longer blocks: 441782
Depends on: 441782
Whiteboard: fix in bug 441782
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 4

10 years ago
FWIW, this regressed between 2007-07-25-04 and 2007-07-26-04, presumably by the changes to ClearStyleDataAndReflow in bug 386640.
(Assignee)

Comment 5

10 years ago
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.
(Reporter)

Comment 6

10 years ago
I think it's basically the same regression as bug 394057.
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).
(Assignee)

Comment 8

10 years ago
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.
Blocks: 386640
Flags: in-testsuite?

Updated

9 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
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).
Flags: wanted1.9.0.x?
(Assignee)

Comment 10

9 years ago
Fixed on mozilla-central by bug 441782.  roc: how can I go about testing this in an automated test?
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(Reporter)

Comment 11

9 years ago
Does this need a separate testcase? IIRC if this regressed layout/base/tests/test_bug441782.html would fail.
(Assignee)

Comment 12

9 years ago
Yes, I think you're right.
Flags: in-testsuite? → in-testsuite-
If this bug is covered in another test, then this is in‑testsuite+ :)
Flags: in-testsuite- → in-testsuite+
(Assignee)

Updated

9 years ago
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.