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

Status: RESOLVED FIXED

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

Description

[Tomer Cohen :tomer]

Attachment: 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

Comment 1

[Tomer Cohen :tomer]

jfkthame, can you please take a look on this issue? Thanks!

Comment 2

[Jonathan Kew [:jfkthame]]

Yes, I can confirm I can reproduce this; will do some debugging...

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: 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

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Ensure we don't continue using a stale BidiDataProperty after dynamic content changes make it obsolete

Comment 5

[Daniel Holbert [: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

Comment 6

[Daniel Holbert [:dholbert]]

(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 7

[Daniel Holbert [:dholbert]]

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

Comment 8

[Jonathan Kew [:jfkthame]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f76c5baf28a
https://hg.mozilla.org/mozilla-central/rev/d90c91b76b8e