1370053 - Bidi reordering regression in gmail compose message

Status: RESOLVED FIXED

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

Description

[Simon Montagu :smontagu]

I didn't manage to create a reduced testcase for this, but I can reliably reproduce it in gmail with the display language set to an RTL language.

Steps to reproduce: 

start to compose a new message in gmail
type a single LTR character in the "To" textbox ("אל" in Hebrew), e.g. J

expected results:
Contacts whose name begins with J appear in a listbox, correctly ordered, with J in boldface

actual results:
The bold J appears at the end of the contacts' names, e.g. "ohn SmithJ"

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e45890951ce77c3df05575bd54072b9f300d77b0&tochange=d700dc054751333e0735f975fce3d3adf153c62a

Comment 1

[Jonathan Kew [:jfkthame]]

Yes, I can reliably reproduce this in gmail, though I haven't come up with a simplified testcase yet.

The problem seems to arise from the code in ResolveParagraph that takes a short-circuit in the case of an empty textframe. I have a patch that I think may fix the issue; I've pushed a try job (https://treeherder.mozilla.org/#/jobs?repo=try&revision=adfce6684ead6efea845ecac0d9821308e972fdf) and will post it for review if that looks OK.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Don't do the empty-frame short-circuit in ResolveParagraph until after updating the current bidi run if necessary

OK, this fixes the bug seen in gmail, and doesn't regress anything else (afaict so far...)

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Fix a couple of comment/variable-name typos in ResolveParagraph

While we're here, a couple of typos caught my eye. (No actual code changes here.)

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Could you add a test for it?

Comment 5

[Jonathan Kew [:jfkthame]]

I'd like to, but so far I haven't come up with a reasonable-sized testcase (i.e. something less than interacting with the Compose panel on a Gmail page!) that reproduces the bug. Simply trying to recreate a similar frame subtree doesn't seem to be sufficient; I think it may be dependent on exactly how it gets dynamically created. (I guess that's not totally surprising, considering that bug 489887 -- where the special handling of an empty text node was originally added -- was about inconsistency after a dynamic change.)

Comment 6

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c474a1264de73553754f9d3570613cf2be594148
Bug 1370053 - Don't do the empty-frame short-circuit in ResolveParagraph until after updating the current bidi run if necessary. r=smontagu

https://hg.mozilla.org/integration/mozilla-inbound/rev/34eb6c45444316f0b9e0a86f0b7a22aa1d3ae86f
Bug 1370053 - Fix a couple of comment/variable-name typos in ResolveParagraph. r=smontagu

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c474a1264de7
https://hg.mozilla.org/mozilla-central/rev/34eb6c454443