1033488 - [e10s][RTL] Do bidi state detection in the parent process on Windows

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P1)

Description

[atifrea]

For information about this bug see bug 966395 and especially the comment at [1].
The issue for OSX has already been fixed.

[1] - https://bugzilla.mozilla.org/show_bug.cgi?id=966395#c10

Comment 1

[Chris Peterson [:cpeterson]]

Moving old M2 P2 bugs to M4.

Comment 2

[Chris Peterson [:cpeterson]]

Move old M2's low-priority bugs to M6 milestone.

Comment 3

[Chris Peterson [:cpeterson]]

oops: old M2 P2 bugs should have been moved to new M4 milestone, not M6.

Comment 5

[Simon Montagu :smontagu]

The duplicate bug 1191895 is a serious user-facing regression for RTL language users, including on OSX, in spite of comment 0 "the issue for OSX has already been fixed". For a while bug 1164693 concealed the problem, but since that was backed out enabling e10s now causes the directional marker on the caret to disappear. See bug 1216096 comment 11 onwards.

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jim, once the backout patches from bug 1216096 will land on aurora and beta, bidi caret marker will be broken in e10s. I am not sure where this is in terms of e10s scenario priority but to me it seems like a Beta blocker i.e. it should be fixed before 50% of beta population gets e10s enabled in 45 or later. Thoughts?

Comment 7

[Jim Mathies [:jimm]]

getting this into triage. sounds like an m8.

Comment 8

[Jim Mathies [:jimm]]

I set bidi.browser.ui to true, but I don't see any change with the caret. Simon, can you provide STR for this bug?

Comment 9

[Jim Mathies [:jimm]]

related, same bug on linux is bug 1033483.

Comment 10

[Simon Montagu :smontagu]

(In reply to Jim Mathies [:jimm] from comment #8)
> I set bidi.browser.ui to true, but I don't see any change with the caret.
> Simon, can you provide STR for this bug?

You also need an installed keyboard driver for a RTL language, e.g. Hebrew or Arabic.

Comment 11

[Jeff Griffiths (:canuckistani) (:⚡︎)]

My initial take on this is, this should be fixed when we ship e10s to the affected locales, eg Hebrew and Arabic. We should also avoid including affected locales in an e10s cohort, don't know if this is possible.

needinfo'ing Pike - do you have a quick list of locales that need bidi in order to be usable?

Comment 12

[Axel Hecht [:Pike]]

"ar fa he ur" are the ones we have these days.

Note, that list is useful for cohort-building, but I'd cringe if we hard-code it in a bunch of places.

Comment 13

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Axel Hecht [:Pike] from comment #12)
> "ar fa he ur" are the ones we have these days.
> 
> Note, that list is useful for cohort-building, but I'd cringe if we
> hard-code it in a bunch of places.

Cohort-building is (I think) what I'm interested in - if we can avoid these locales for the beta e10s cohorts then it affects the priority/milestone of the bug.

Comment 14

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Having thought about this some more, my proposal is that this issue should block testing / deployment of e10s to these locales: ar, fa, he, ur

I'm clearing my needinfo, and needinfo'ing Jim so this comes up in triage again and prioritized. I don't think this is a high priority for 46, but it should get fixed.

Comment 15

[Jim Mathies [:jimm]]

removing my ni so this falls into our triage list. lets find out how hard this will be to fix. We can file a bug on restricting e10s rollout based on locale if need be.

Comment 16

[:Felipe Gomes (needinfo for replies!)]

(In reply to Axel Hecht [:Pike] from comment #12)
> "ar fa he ur" are the ones we have these days.
> 
> Note, that list is useful for cohort-building, but I'd cringe if we
> hard-code it in a bunch of places.

Hey Axel, what is ur? or did you mean ru?

Comment 17

[Francesco Lodolo [:flod]]

ur = Urdu
https://en.wikipedia.org/wiki/Urdu

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Ok thanks. Out of curiosity, is there a file somewhere that lists all the possible locales? I tried looking for it, and there are various files called "all-locales" on the tree, but they are all pretty incomplete.

I wanted to know if any of these locales also have a "-AA" variant or if they always come with just these two letters

Comment 19

[Francesco Lodolo [:flod]]

I'm not sure if there's a comprehensive list around, but the all-locales for DevEdition should cover most of them
http://hg.mozilla.org/releases/mozilla-aurora/file/tip/browser/locales/all-locales

Locale codes with language and region (ab-CD) are quite common, e.g. we have 4 Spanish regional variants.

There are also some variants in Firefox OS world (e.g. sr-Latn, sr-Cyrl for Serbian, es for Spanish), but I don't think you're interested in those.

This is a JSON of all locale code+description available in the Mozilla Localizations component
https://l10n.mozilla-community.org/~flod/mozilla-l10n-query/?bugzilla=product

Comment 20

[Makoto Kato [:m_kato]]

Attachment: MozReview Request: Bug 1033488 - Send RTL information to child process by WM_INPUTLANGCHANGE. r?masayuki

Review commit: https://reviewboard.mozilla.org/r/39441/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39441/

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8729402 [details]
MozReview Request: Bug 1033488 - Send RTL information to child process by WM_INPUTLANGCHANGE. r?masayuki

https://reviewboard.mozilla.org/r/39441/#review36125

I think that you should use nsBidiKeyboard::OnLayoutChange() from IMEInputHandler::OnCurrentTextInputSourceChange() too, though.

# Wow, sorry for the delay due to fogetting to click "Publish Review"... Sigh.

Comment 22

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #21)
> Comment on attachment 8729402 [details]
> MozReview Request: Bug 1033488 - Send RTL information to child process by
> WM_INPUTLANGCHANGE. r?masayuki
> 
> https://reviewboard.mozilla.org/r/39441/#review36125
> 
> I think that you should use nsBidiKeyboard::OnLayoutChange() from
> IMEInputHandler::OnCurrentTextInputSourceChange() too, though.
> 
> # Wow, sorry for the delay due to fogetting to click "Publish Review"...
> Sigh.

Same issue on OSX is already fixed by bug 966395.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Makoto Kato [:m_kato] from comment #22)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #21)
> > Comment on attachment 8729402 [details]
> > MozReview Request: Bug 1033488 - Send RTL information to child process by
> > WM_INPUTLANGCHANGE. r?masayuki
> > 
> > https://reviewboard.mozilla.org/r/39441/#review36125
> > 
> > I think that you should use nsBidiKeyboard::OnLayoutChange() from
> > IMEInputHandler::OnCurrentTextInputSourceChange() too, though.
> > 
> > # Wow, sorry for the delay due to fogetting to click "Publish Review"...
> > Sigh.
> 
> Same issue on OSX is already fixed by bug 966395.

I meant that you add a useful method in the patch, therefore, IMEInputHandler should use it too.

Comment 24

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #23)
> (In reply to Makoto Kato [:m_kato] from comment #22)
> > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #21)
> > > Comment on attachment 8729402 [details]
> > > MozReview Request: Bug 1033488 - Send RTL information to child process by
> > > WM_INPUTLANGCHANGE. r?masayuki
> > > 
> > > https://reviewboard.mozilla.org/r/39441/#review36125
> > > 
> > > I think that you should use nsBidiKeyboard::OnLayoutChange() from
> > > IMEInputHandler::OnCurrentTextInputSourceChange() too, though.
> > > 
> > > # Wow, sorry for the delay due to fogetting to click "Publish Review"...
> > > Sigh.
> > 
> > Same issue on OSX is already fixed by bug 966395.
> 
> I meant that you add a useful method in the patch, therefore,
> IMEInputHandler should use it too.

This patch is for widget/windows.  Do you mean that I should create widget/nsBaseBidiKeyboard class too or I should add same method to widget/cocoa/nsBidiKeyboard.mm?

Comment 25

[Makoto Kato [:m_kato]]

Or I should add same method to WidgetUtils for all platforms?

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Makoto Kato [:m_kato] from comment #24)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #23)
> > (In reply to Makoto Kato [:m_kato] from comment #22)
> > > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #21)
> > > > Comment on attachment 8729402 [details]
> > > > MozReview Request: Bug 1033488 - Send RTL information to child process by
> > > > WM_INPUTLANGCHANGE. r?masayuki
> > > > 
> > > > https://reviewboard.mozilla.org/r/39441/#review36125
> > > > 
> > > > I think that you should use nsBidiKeyboard::OnLayoutChange() from
> > > > IMEInputHandler::OnCurrentTextInputSourceChange() too, though.
> > > > 
> > > > # Wow, sorry for the delay due to fogetting to click "Publish Review"...
> > > > Sigh.
> > > 
> > > Same issue on OSX is already fixed by bug 966395.
> > 
> > I meant that you add a useful method in the patch, therefore,
> > IMEInputHandler should use it too.
> 
> This patch is for widget/windows.  Do you mean that I should create
> widget/nsBaseBidiKeyboard class too or I should add same method to
> widget/cocoa/nsBidiKeyboard.mm?

Ah, sorry. I misunderstood. I understood as you adding OnLayoutChange() into XP level class. But not so actually. So, ignore my comments. Sorry for the confusion.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Makoto Kato [:m_kato] from comment #25)
> Or I should add same method to WidgetUtils for all platforms?

It might be better. But it's okay not to do it here (there are more urgent bugs...)

Comment 28

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #27)
> (In reply to Makoto Kato [:m_kato] from comment #25)
> > Or I should add same method to WidgetUtils for all platforms?
> 
> It might be better. But it's okay not to do it here (there are more urgent
> bugs...)

I will create the follow up bugs for Cocoa and Windows.  Also it will be useful for bug 1033483.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd788a05e4dc

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bd788a05e4dc

Comment 31

[blinky]

When I switching en to ar, direction cursor does not change.

Screenshot https://dl.dropboxusercontent.com/u/95157096/85f61cf7/t561exsbt3.png

Comment 32

[Makoto Kato [:m_kato]]

(In reply to blinky from comment #31)
> When I switching en to ar, direction cursor does not change.
> 
> Screenshot
> https://dl.dropboxusercontent.com/u/95157096/85f61cf7/t561exsbt3.png

Are you using local build binary that you build from the latest source code of m-c or m-i?

Comment 33

[Makoto Kato [:m_kato]]

Ah, bug 966395's fix is invalid too.   Chrome process can send RTL information to content process correctly.  But content process doesn't have bidi keyboard even if PuppetBidiKeyboard is implemented!  I will fix it soon.

Comment 34

[Makoto Kato [:m_kato]]

Attachment: Part 2. Make PuppetBidiKeyboard correctly on content process.

Actually, content process cannot get nsIBidiKeyboard by do_GetService("@mozilla.org/widget/bidikeyboard") on OSX and Windows.  Since ContentChild will call PuppetBidiKeybaord directly, we should always register nsIBidiKeyboard by PuppetBidiKeyboard.

Comment 35

[blinky]

(In reply to Makoto Kato [:m_kato] from comment #32)
> (In reply to blinky from comment #31)
> > When I switching en to ar, direction cursor does not change.
> > 
> > Screenshot
> > https://dl.dropboxusercontent.com/u/95157096/85f61cf7/t561exsbt3.png
> 
> Are you using local build binary that you build from the latest source code
> of m-c or m-i?

I am using the latest nightly m-c build.

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/548a188a8737

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/548a188a8737

Comment 38

[Makoto Kato [:m_kato]]

Comment on attachment 8729402 [details]
MozReview Request: Bug 1033488 - Send RTL information to child process by WM_INPUTLANGCHANGE. r?masayuki

Approval Request Comment
[Feature/regressing bug #]:
Nothing

[User impact if declined]:
RTL caret doesn't show correctly on e10s mode with bidi.browser.ui=true.

Due to this issue, we don't turn on e10s on RTL languages.

[Describe test coverage new/current, TreeHerder]:
Landed in m-c

[Risks and why]:
too low.  This only works on bidi.browser.ui=true and e10s when keyboard layout is changed by user.

[String/UUID change made/needed]:
Nothing

Comment 39

[Makoto Kato [:m_kato]]

Comment on attachment 8732052 [details] [diff] [review]
Part 2. Make PuppetBidiKeyboard correctly on content process.

Approval Request Comment
[Feature/regressing bug #]:
Nothing

[User impact if declined]:
RTL caret doesn't show correctly on e10s mode with bidi.browser.ui=true.

Due to this issue, we don't turn on e10s on RTL languages and this is required for OSX too.

[Describe test coverage new/current, TreeHerder]:
Landed in m-c

[Risks and why]:
too low.  This only works on bidi.browser.ui=true and e10s when keyboard layout is changed by user.

[String/UUID change made/needed]:
Nothing

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8729402 [details]
MozReview Request: Bug 1033488 - Send RTL information to child process by WM_INPUTLANGCHANGE. r?masayuki

e10s + bidi, Aurora47+

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c8d83343c57b
https://hg.mozilla.org/releases/mozilla-aurora/rev/7531defe6cb7

Comment 42

[Jim Mathies [:jimm]]

Hey Makoto, with this bug fixed can we turn e10s on for rtl users on windows?

Comment 43

[Makoto Kato [:m_kato]]

(In reply to Jim Mathies [:jimm] from comment #42)
> Hey Makoto, with this bug fixed can we turn e10s on for rtl users on windows?

Yes.  RTL caret and context menu for RTL work fine even if on e10s.

But I don't know whether bug 1257731 is critical as Firefox team.  But I think that this isn't critical because direction can change by context menu or another shortcut (accel+shift+x).

Comment 44

[Jim Mathies [:jimm]]

(In reply to Makoto Kato [:m_kato] from comment #43)
> (In reply to Jim Mathies [:jimm] from comment #42)
> > Hey Makoto, with this bug fixed can we turn e10s on for rtl users on windows?
> 
> Yes.  RTL caret and context menu for RTL work fine even if on e10s.
> 
> But I don't know whether bug 1257731 is critical as Firefox team.  But I
> think that this isn't critical because direction can change by context menu
> or another shortcut (accel+shift+x).

Looks like you'll have that fixed in 50 so lets just target that release. I'll file a bug.