Switch to IME of jsyinpin or jszhuyin, the period right aside space key, there is no alternate indicator. Lack of some style sheet.

Comment on attachment 8515781 [details] Patch - pull request 25744 Hi Tim, Do you have time take a look? It's a small patch.

There are two if statements, if (overwrites['.'] !== false) { if (overwrites['.']) { I have no idea why we need two, one is OK I think, do you know some reasons for it? Thanks！ (In reply to wdeng@mozilla.com from comment #2) > Comment on attachment 8515781 [details] > Patch - pull request 25744 > > Hi Tim, > Do you have time take a look? It's a small patch.

Comment on attachment 8515781 [details] Patch - pull request 25744 I don't think this is the proper fix -- we should just add 'alternate-indicator' to overwrite key object.

:mnjul, do you think it make sense to add 'alternate-indicator' from LayoutNormalizer#_normalizeKey ?

Well...it does appear that the addition of alternate-indicator is not quite robust currently. For example, for Polish keyboard comma key "," has alternative chars [1] but layout manager would not add an indicator for it. We should be more fail-safe on this. IMO, to correctly address the issue, we should do both of: 1. For every hard-coded key in LayoutManager#_updateCurrentPage, we should check if the key has alt chars defined in that page for it, and add alternate-indicator accordingly. This fixes the Polish KB. 2. Try to add the alternate-indicator className in |LayoutNormalizer#_normalizeKey| whenever the normalizer deems that the current normalized key has alt chars defined in the current normalized page for it. Since overwrite chars will also be normalized by this function, this fixes the bug in comment 0. This would align things together more ;) Additionally, instead of adding "className: alternate-indicator" property in key objects, we should consider only adding a flag (maybe "alternativeIndicator: true" ?) and let render deal with DOM classNames accordingly, as we would want to decouple view logics from layout definitions more.

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #6) > Well...it does appear that the addition of alternate-indicator is not quite > robust currently. For example, for Polish keyboard comma key "," has > alternative chars [1] but layout manager would not add an indicator for it. > We should be more fail-safe on this. > > IMO, to correctly address the issue, we should do both of: > 1. For every hard-coded key in LayoutManager#_updateCurrentPage, we should > check if the key has alt chars defined in that page for it, and add > alternate-indicator accordingly. This fixes the Polish KB. > 2. Try to add the alternate-indicator className in > |LayoutNormalizer#_normalizeKey| whenever the normalizer deems that the > current normalized key has alt chars defined in the current normalized page > for it. Since overwrite chars will also be normalized by this function, this > fixes the bug in comment 0. Besides overwrite chars, all the chars in page array are also be normalized by this function. It means, any char in a page which is also a key in alt object, would be added "alternate-indicator" className. I don't think that's what we want. Am I wrong? Or do you mean, we should ignore "alternate-indicator" for the chars in page and only check chars in overwrite object? > > This would align things together more ;) > > Additionally, instead of adding "className: alternate-indicator" property in > key objects, we should consider only adding a flag (maybe > "alternativeIndicator: true" ?) and let render deal with DOM classNames > accordingly, as we would want to decouple view logics from layout > definitions more.

(In reply to wdeng@mozilla.com from comment #7) > (In reply to John Lu [:mnjul] [MoCoTPE] from comment #6) > > Well...it does appear that the addition of alternate-indicator is not quite > > robust currently. For example, for Polish keyboard comma key "," has > > alternative chars [1] but layout manager would not add an indicator for it. > > We should be more fail-safe on this. > > > > IMO, to correctly address the issue, we should do both of: > > 1. For every hard-coded key in LayoutManager#_updateCurrentPage, we should > > check if the key has alt chars defined in that page for it, and add > > alternate-indicator accordingly. This fixes the Polish KB. > > 2. Try to add the alternate-indicator className in > > |LayoutNormalizer#_normalizeKey| whenever the normalizer deems that the > > current normalized key has alt chars defined in the current normalized page > > for it. Since overwrite chars will also be normalized by this function, this > > fixes the bug in comment 0. > > Besides overwrite chars, all the chars in page array are also be > normalized by this function. It means, any char in a page which is also a > key in alt object, would be added "alternate-indicator" className. I don't > think that's what we want. Am I wrong? > > Or do you mean, we should ignore "alternate-indicator" for the chars in > page and only check chars in overwrite object? > > > > > This would align things together more ;) > > > > Additionally, instead of adding "className: alternate-indicator" property in > > key objects, we should consider only adding a flag (maybe > > "alternativeIndicator: true" ?) and let render deal with DOM classNames > > accordingly, as we would want to decouple view logics from layout > > definitions more. Hmm.. :/ You're right. My proposal was wrong. I am afraid that even adding "alternate-indicator" only for overwriting chars (that have alts) in |normalizeKey| would not really work, as in the future there could be some overwritten char who does not originally have "alternate-indicator" despite having alt chars by itself. (Or we can argue that this is invalid assumption) I think we probably still need to go the route "If overwritten char X has |alternate-indicator|, then the overwriting char X should get this property too". i.e. something like this around [1]: if (overwrites['.']) { var overwritingPeriodKey = Object.create(overwrites['.']); overwritingPeriodKey.className = periodKey.className; periodKey = overwritingPeriodKey; } Note the Object.create to not alter the original key object, like what we do for enterKeyObject and spaceKeyObject. Of course, adding alternate-indicator like [2] should also be done for the commaKey, for the above logics and Polish keyboard to work. Tim, how do you think about this? This seems an inherent issue with the fact that we generate hard-coded key objects (with complex logics) in LayoutManager#_updateCurrentPage, though. :/ [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/layout_manager.js#L396 [2] https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/layout_manager.js#L284-L286

(for comment 8)

I found that, some keys in layout.pages, like in layout/hi.js etc.. { value: '₹', className: 'alternate-indicator' } It means we can show indicator for any key which is defined in layout. And in layoutManager._updateCurrentPage function, only IME switch key and period key could be added "alternate-indicator" style, the two keys are created in the function, we don't define them in layout. I think wen can still leave IME switch key there, and check period key in LayoutNormalizer, a) whether textLayoutOverwrite contains period key, and b) whether period key is in alt object. but it's still hard code. In my own opinion, if we can define or customize className for textLayoutOverwrite just as other keys in pages, the logic will be much simpler. (In reply to John Lu [:mnjul] [MoCoTPE] from comment #8) > (In reply to wdeng@mozilla.com from comment #7) > > (In reply to John Lu [:mnjul] [MoCoTPE] from comment #6) > > > Well...it does appear that the addition of alternate-indicator is not quite > > > robust currently. For example, for Polish keyboard comma key "," has > > > alternative chars [1] but layout manager would not add an indicator for it. > > > We should be more fail-safe on this. > > > > > > IMO, to correctly address the issue, we should do both of: > > > 1. For every hard-coded key in LayoutManager#_updateCurrentPage, we should > > > check if the key has alt chars defined in that page for it, and add > > > alternate-indicator accordingly. This fixes the Polish KB. > > > 2. Try to add the alternate-indicator className in > > > |LayoutNormalizer#_normalizeKey| whenever the normalizer deems that the > > > current normalized key has alt chars defined in the current normalized page > > > for it. Since overwrite chars will also be normalized by this function, this > > > fixes the bug in comment 0. > > > > Besides overwrite chars, all the chars in page array are also be > > normalized by this function. It means, any char in a page which is also a > > key in alt object, would be added "alternate-indicator" className. I don't > > think that's what we want. Am I wrong? > > > > Or do you mean, we should ignore "alternate-indicator" for the chars in > > page and only check chars in overwrite object? > > > > > > > > This would align things together more ;) > > > > > > Additionally, instead of adding "className: alternate-indicator" property in > > > key objects, we should consider only adding a flag (maybe > > > "alternativeIndicator: true" ?) and let render deal with DOM classNames > > > accordingly, as we would want to decouple view logics from layout > > > definitions more. > > Hmm.. :/ You're right. My proposal was wrong. > > I am afraid that even adding "alternate-indicator" only for overwriting > chars (that have alts) in |normalizeKey| would not really work, as in the > future there could be some overwritten char who does not originally have > "alternate-indicator" despite having alt chars by itself. (Or we can argue > that this is invalid assumption) > > I think we probably still need to go the route "If overwritten char X has > |alternate-indicator|, then the overwriting char X should get this property > too". i.e. something like this around [1]: > > if (overwrites['.']) { > var overwritingPeriodKey = Object.create(overwrites['.']); > overwritingPeriodKey.className = periodKey.className; > periodKey = overwritingPeriodKey; > } > > Note the Object.create to not alter the original key object, like what we do > for enterKeyObject and spaceKeyObject. > > Of course, adding alternate-indicator like [2] should also be done for the > commaKey, for the above logics and Polish keyboard to work. > > Tim, how do you think about this? This seems an inherent issue with the fact > that we generate hard-coded key objects (with complex logics) in > LayoutManager#_updateCurrentPage, though. :/ > > [1] > https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/ > layout_manager.js#L396 > [2] > https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/ > layout_manager.js#L284-L286

Per comment 10 and offline discussion, let's adopt something similar to comment 10 suggests: Update LayoutNormalizer#_normalizeKey so it would apply a different normalizing rule, and only when the key-to-normalize comes from textLayoutOverwrite we will add the className.

Comment on attachment 8515781 [details] Patch - pull request 25744 Hi Tim, The pull request has been updated, do you have time take a look? Thanks!

Comment on attachment 8515781 [details] Patch - pull request 25744 Looks good.

Comment on attachment 8515781 [details] Patch - pull request 25744 Hi Tim, Modified some variable and argument names and it passed all try-server tests. Thanks for your review.

Comment on attachment 8515781 [details] Patch - pull request 25744 The patch looks good -- however it lack tests. The fact that the change does not make any current tests to fail is a bit worrying, but that's probably acceptable. :-/ Could you talk to :mnjul to add a few tests to the normalizeKey function?

OK, I will communicate with mnjul, and it maybe delayed to next week, something emergency to do this week. Thank you. (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #15) > Comment on attachment 8515781 [details] > Patch - pull request 25744 > > The patch looks good -- however it lack tests. The fact that the change does > not make any current tests to fail is a bit worrying, but that's probably > acceptable. :-/ > > Could you talk to :mnjul to add a few tests to the normalizeKey function?

Let's add a test for |_normalizeKey| that checks if |showAlternatesIndicator| parameter is correctly honored, and a test for |_normalizePageKeys| that checks if the |hasAlternativeKeys| variable is correctly derived from |alt| & |overwrites| from a page (and is propagated into |_normalizeKey|); this test should utilize |_hasAlternativeKeys|. I think there is no need to separately test for |_hasAlternativeKeys|.

Thank you John, I will do it ASAP. (In reply to John Lu [:mnjul] [MoCoTPE] from comment #17) > Let's add a test for |_normalizeKey| that checks if > |showAlternatesIndicator| parameter is correctly honored, and a test for > |_normalizePageKeys| that checks if the |hasAlternativeKeys| variable is > correctly derived from |alt| & |overwrites| from a page (and is propagated > into |_normalizeKey|); this test should utilize |_hasAlternativeKeys|. I > think there is no need to separately test for |_hasAlternativeKeys|.

Hi John, Added some test cases following your suggestions. Do you have time take a look? Thanks.

I've just looked at the tests and they look pretty good to me. Nice work!

Thank you John.

Comment on attachment 8515781 [details] Patch - pull request 25744 Woot!

master: https://github.com/mozilla-b2g/gaia/commit/18a63a1bb3fd4f8b062d4daf2dc04dd7befa076a