bidi failure causing reversed LTR text if there was presence of RTL text

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout: Text
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: tomer, Assigned: jfkthame)

Tracking

({regression, rtl})

55 Branch
mozilla55
regression, rtl
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

7 months ago
Created attachment 8869872 [details]
Demonstration video

Sometimes I forget to switch my keyboard layout to the right language before starting to type. Recently it started causing the content to appear reversed, as if there is an hidden RLO character that causing the LTR content to be typed reversed. 

Steps to reproduce:
Here I'm trying to simulate what happens when I try to write the word 'Example' while my keyboard is set to Hebrew

1. Set language to Hebrew.
2. Press shift-e in order to type uppercase E
3. Continue typing 'xample' in lowercase, then notice the wrong layout is used. 
4. Delete with backspace (or mouse selection) the bad text, but keep the uppercase E at the begging. 
5. Switch keyboard layout to english.
6. type again 'xample' in lowercase, notice that the text appears reversed.

Actual result:
The word 'Example' appears as 'Eelpmax'. See attached video.



MozRegression outout:
12:35.23 INFO: Last good revision: 0b77ed3f26c5335503bc16e85b8c067382e7bb1e
12:35.23 INFO: First bad revision: c0d35b1c5ab5fa9bb2f5661aa0454a1ce31b50e0
12:35.23 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b77ed3f26c5335503bc16e85b8c067382e7bb1e&tochange=c0d35b1c5ab5fa9bb2f5661aa0454a1ce31b50e0

It appears that it is a regression caused by one of the following commits made by :jfkthame - 
	2318d492b17f	Jonathan Kew — Bug 1359857 - For frames that have a BidiDataProperty, include this in the debugging info printed by ListGeneric. r=dholbert
	dba6cd251066	Jonathan Kew — Bug 1358548 - Move functionality of BidiParagraphData::Init() into a constructor instead of a separate method. r=dholbert
	033145d32313	Jonathan Kew — Bug 1358275 - Optimize away repeated StyleContext() calls in nsBidiPresUtils::TraverseFrames. rs=dholbert
	779ea6294a22	Jonathan Kew — Bug 1358275 - When no BidiDataProperty() is present, nsIFrame::GetBidiData() should return data with precedingControl=kBidiLevelNone to avoid unecessarily interrupting textruns. r=dholbert
	155ba7301e59	Jonathan Kew — Bug 1358275 - Skip the main body of bidi-resolution for blocks that can be determined to be purely LTR content without directional overrides/embeddings. r=dholbert
	99440ee674fe	Jonathan Kew — Bug 1358275 - Preliminary cleanup: refactor nsTextFragment::UpdateBidiFlag(), moving its logic into nsBidiUtils function HasRTLChars() and making that function more precise in the process. r=dholbert
(Reporter)

Comment 1

7 months ago
jfkthame, can you please take a look on this issue? Thanks!
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(jfkthame)

Updated

7 months ago
Keywords: regression
(Assignee)

Comment 2

7 months ago
Yes, I can confirm I can reproduce this; will do some debugging...
Assignee: nobody → jfkthame
Flags: needinfo?(jfkthame)
(Assignee)

Comment 3

7 months ago
Created attachment 8870049 [details] [diff] [review]
Reftest based on the testcase here. Fails with current trunk code, because a frame that formerly required bidi resolution (and so got a BidiDataProperty attached) no longer has any bidi content, hence we skip bidi resolution and the now-obsolete property
Attachment #8870049 - Flags: review?(dholbert)
(Assignee)

Comment 4

7 months ago
Created attachment 8870050 [details] [diff] [review]
Ensure we don't continue using a stale BidiDataProperty after dynamic content changes make it obsolete
Attachment #8870050 - Flags: review?(dholbert)
Comment on attachment 8870049 [details] [diff] [review]
Reftest based on the testcase here. Fails with current trunk code, because a frame that formerly required bidi resolution (and so got a BidiDataProperty attached) no longer has any bidi content, hence we skip bidi resolution and the now-obsolete property

Review of attachment 8870049 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8870049 - Attachment description: Reftest based on the testcase here. Fails with current trunk code, because a frame that formerly required bidi resolution (and so got a BidiDataProperty attached) no longer has any bidi content, hence we skip bidi resolution and the now-obsolete property → Reftest based on the testcase here. Fails with current trunk code, because a frame that formerly required bidi resolution (and so got a BidiDataProperty attached) no longer has any bidi content, hence we skip bidi resolution and the now-obsolete property
Attachment #8870049 - Flags: review?(dholbert) → review+
(Odd; apparently Bugzilla thinks I changed the title of the attachment in my previous tweak, though I didn't make any intentional tweak and its "old"/"new" text look the same to me, & are the same according to a merge tool.)
Comment on attachment 8870050 [details] [diff] [review]
Ensure we don't continue using a stale BidiDataProperty after dynamic content changes make it obsolete

Review of attachment 8870050 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8870050 - Flags: review?(dholbert) → review+
(Assignee)

Comment 8

7 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f76c5baf28aba6205273330b1f10321aae29cec
Bug 1366623 - Reftest for reflow of a frame that formerly required bidi resolution (and so got a BidiDataProperty attached) but no longer has any bidi content, so the previously-attached property sticks has become obsolete. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/d90c91b76b8ebc76805599e39bc6cec681260add
Bug 1366623 - Ensure we don't continue using a stale BidiDataProperty after dynamic content changes make it obsolete. r=dholbert

Comment 9

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5f76c5baf28a
https://hg.mozilla.org/mozilla-central/rev/d90c91b76b8e
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.