Closed Bug 435856 Opened 12 years ago Closed 12 years ago

LTR overwrite after printing a page

Categories

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

1.9.0 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: zwnj, Assigned: smontagu)

References

(Blocks 2 open bugs, )

Details

(4 keywords, Whiteboard: [RC2-])

Attachments

(6 files)

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.
Does this bug really block releasing a Persian version of Firefox? As that is what blocking bug 415597 indicates.
Also I should note that it doesn't depend on the printing device.  Both native PS/PDF and CUPS-PDF trigger the problem.
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.
Flags: blocking1.9?
Flags: blocking1.9.0.1?
OS: Linux → All
Hardware: PC → All
Version: unspecified → 1.9.0 Branch
> 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".
Flags: blocking1.9?
Flags: blocking1.9.0.1?
Attached file Test case
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.
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.
Keywords: regression, testcase
(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.  :-)
Flags: blocking1.9?
Flags: blocking1.9.0.1?
Whiteboard: [RC2?]
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.
Flags: blocking1.9? → blocking1.9-
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.)
Flags: wanted1.9.0.x+
Flags: in-testsuite?
Attached patch PatchSplinter Review
This is my regression from bug 421690.
Assignee: nobody → smontagu
Status: NEW → ASSIGNED
Attachment #322663 - Flags: superreview?
Attachment #322663 - Flags: review?
Attachment #322663 - Flags: superreview?(roc)
Attachment #322663 - Flags: superreview?
Attachment #322663 - Flags: review?(roc)
Attachment #322663 - Flags: review?
Comment on attachment 322663 [details] [diff] [review]
Patch

Maybe it would be better to enforce this in the setter? Or assert there at least?
Whiteboard: [RC2?] → [RC2-]
No longer blocks: fx35-l10n-fa
Attachment #322663 - Flags: superreview?(roc)
Attachment #322663 - Flags: superreview+
Attachment #322663 - Flags: review?(roc)
Attachment #322663 - Flags: review+
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.
Attachment 322663 [details] [diff] checked in to mozilla-central.
Attachment #322663 - Flags: approval1.9?
Attachment #325149 - Flags: superreview?(roc)
Attachment #325149 - Flags: review?(roc)
Attachment #325149 - Flags: superreview?(roc)
Attachment #325149 - Flags: superreview+
Attachment #325149 - Flags: review?(roc)
Attachment #325149 - Flags: review+
Flags: blocking1.9.0.1? → blocking1.9.0.1-
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.
Attachment #322663 - Flags: approval1.9? → approval1.9.0.1?
Blocks: 438975
Should have marked this FIXED before; both patches were checked in to mozilla-central already.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #322663 - Flags: approval1.9.0.1? → approval1.9.0.2?
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.
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.
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.
Attached patch Combined patchSplinter Review
Attachment #330482 - Flags: superreview+
Attachment #330482 - Flags: review+
Attachment #330482 - Flags: approval1.9.0.2?
Attachment #322663 - Flags: approval1.9.0.2?
Comment on attachment 330482 [details] [diff] [review]
Combined patch

Approved for 1.9.0.2. Please land in CVS. a=ss
Attachment #330482 - Flags: approval1.9.0.2? → approval1.9.0.2+
Keywords: fixed1.9.0.2
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.