Behnam Esfahbod [:zwnj] Reporter
	Description • 16 years ago

After printing a page with RTL content, every style change in RTL text makes a weird change in text render, that's like overwriting bidi direction.

Here are some facts:

- The content has no problem, ie. if you copy it and paste elsewhere, you'll get the right content;

- After some weird changes, ie. hovering some links, and printing again, there won't be any change in the printed PS.

- And the important one, joining algorithm is applied in new direction, which means ALEF is still joining to its right character, although the right character is logically after the ALEF in the content.

- I found this problem many weeks ago, and probably it was there after rewriting text render module in 1.9.


This bug do effect all pages with RTL content.  So I'm asking for blocking1.9.  Hopefully it gets wanted1.9.

I'm going to attach some screenshots to make it clear.

	Axel Hecht [:Pike]
	Comment 1 • 16 years ago

Does this bug really block releasing a Persian version of Firefox? As that is what blocking bug 415597 indicates.

	Behnam Esfahbod [:zwnj] Reporter
	Comment 2 • 16 years ago

Also I should note that it doesn't depend on the printing device.  Both native PS/PDF and CUPS-PDF trigger the problem.

	(no longer active)
	Comment 4 • 16 years ago

Wow, this looks disastrous for RTL content.  CCing Tomer.

I could reproduce the problem on Windows as well in Fx3.0 RC1.  The joining algorithm is not being applied on Windows.

This happens on both printing, and invoking the print preview page.  Note that this does not happen consistently, you sometimes have to hover elements, select text randomly, etc.

I'm preparing a minimal testcase and I'll attach it here.  Requesting blocking1.9 and blocking1.9.0.1 in hopes of getting some attention on this bug.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 5 • 16 years ago

> I'm preparing a minimal testcase and I'll attach it here.  Requesting
> blocking1.9 and blocking1.9.0.1 in hopes of getting some attention on this bug.

This is bug abuse. Only nominate if you believe that the bug should actually block the release. To get attention, file it in the right component (done) and the module owners should see the bugmail.

Clearing flags on the presumption that it was just to get "attention".

	(no longer active)
	Comment 6 • 16 years ago

Minimal test case.  I generated this by visiting <http://www.google.com/webhp?hl=fa> and taking its source and removing as much as possible from the content.

STR:
1. Load the test case.
2. Invoke print preview. (or print the page).
3. Right-click one of the two top links.  The direction of the first div is affected.
4. Hover the right-most link in the second div.  The direction of the second fiv is affected.

	(no longer active)
	Comment 7 • 16 years ago

Note that when the test case is loaded in an iframe (for example: <https://bugzilla.mozilla.org/attachment.cgi?id=322630&action=edit>, the problem doesn't happen.

	(no longer active)
	Comment 8 • 16 years ago

(In reply to comment #5)
> This is bug abuse. Only nominate if you believe that the bug should actually
> block the release. To get attention, file it in the right component (done) and
> the module owners should see the bugmail.
> 
> Clearing flags on the presumption that it was just to get "attention".

Actually, this is quite a big problem for RTL content (and see the test case, even positioning might be affected), that was the reason I requested the blocking flags (in order to get some attention _in the blockers list_).  Therefore, I'm renominating.

I hope it's OK to set both blocking1.9 and blocking1.9.0.1, because I believe this should be considered for RC2 if we decide to have it, or 1.9.0.1 in case 1.9rc1 becomes final.

Sorry for the wrong impression here.  :-)

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 9 • 16 years ago

If you reload the page, does it go away? I don't think I'd even hold an RC2 for this, but we'll keep it on the radar for 1.9.0.1.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 10 • 16 years ago

The obvious workaround (reloading the page) works, not a stop-ship issue.  We should try to get a fix for 3.0.1.  (There's no patch for which to evaluate risk, and so we don't know what our test coverage is on code that would be effected.  Not suitable for rushing into this vehicle; 3.0.1 isn't long, and if we get a fix soon it should be a shoo-in.)

	Simon Montagu :smontagu Assignee
	Comment 11 • 16 years ago

This is my regression from bug 421690.

	Simon Montagu :smontagu Assignee
	Comment 12 • 16 years ago

Comment on attachment 322663 [details] [diff] [review]
Patch

Maybe it would be better to enforce this in the setter? Or assert there at least?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 13 • 16 years ago

Hahaha, what a great bug.

We should definitely take the patch here as soon as we can, risk is zero, benefit significant.

I think SetBidiEnabled shouldn't take a parameter. I can't imagine that we could ever realistically set it to false. Followup patch for that gratefully accepted.

	Simon Montagu :smontagu Assignee
	Comment 14 • 16 years ago

Attachment 322663 [details] [diff] checked in to mozilla-central.

	Simon Montagu :smontagu Assignee
	Comment 16 • 16 years ago

Comment on attachment 322663 [details] [diff] [review]
Patch

Requesting approval1.9.0.1. This is a minimal-risk fix for a well-understood regression and the patch has been baking on the trunk since the beginning of last week.

	Simon Montagu :smontagu Assignee
	Comment 17 • 16 years ago

Should have marked this FIXED before; both patches were checked in to mozilla-central already.

	Samuel Sidler (old account; do not CC)
	Comment 18 • 16 years ago

Simon, can you please create a combined patch with both the original patch and the follow up and request 1.9.0.2 approval on it? We've been bitten in the past by leaving follow up patches behind.

	Simon Montagu :smontagu Assignee
	Comment 19 • 16 years ago

I can do that, but I was assuming that we would only take the original patch for 1.9.0.2, as the minimal change that fixes the bug. On the other hand, I don't think the combined patch would be any higher-risk than the original so maybe we should just go for it.

	Samuel Sidler (old account; do not CC)
	Comment 20 • 16 years ago

Whichever you prefer. I'm more than happy to approve the original patch, I just assumed you'd want the follow up patch as well.

	Samuel Sidler (old account; do not CC)
	Comment 22 • 16 years ago

Comment on attachment 330482 [details] [diff] [review]
Combined patch

Approved for 1.9.0.2. Please land in CVS. a=ss