Merge the rendering of upper case layout and lower case layout

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

The layout rendering of different cases is not the same, and it is distinguished by |flag.uppercase| we passed to IMERender.draw() function. In fact, this is only matters for layout definitions where |secondLayout| is true.

In this bug, I propose we merge the different rendering (a <div class="keyboard-type-container">) of different cases, that should allow us to remove special |secondLayout| handling in RenderingManager, and prevent |showUpperCase| from being needed in VisualHighlightManager.

This has a dependency to bug 985853 because of bug 985853 comment 10. If we keep using different set of DOM elements to render upper/lower case layouts, bug 985853 will not work in layout definitions where |secondLayout| is true.
Comment on attachment 8508773 [details] [review]
mozilla-b2g:master PR#25374

Rudy, John, could you review this? I think having one of you to review the patch would be enough.

With this patch I removed all |secondLayout| handling in business logic, and pass the responsibility to view, since this property affects representation only. Get rid of that simplifies business logic and avoid yet another special handling in bug 985853.

It is undesirable to put more code into render.js, but I want to block bug 985853 as soon as possible so I choose not to spend time factoring out KeyView etc. here. I also didn't pay attention to arguments passed to buildKey() because I want to keep the change at the minim.

Thanks.
Attachment #8508773 - Flags: review?(rlu)
Attachment #8508773 - Flags: review?(jlu)
Comment on attachment 8508773 [details] [review]
mozilla-b2g:master PR#25374

The patch itself looks good but unfortunately I'm experiencing a bug on Japanese IME with it.

STR:
1. Build with Japanese IME support (jp-kanji) and enable it.
2. Open Messages app. Start a new message. (You'll be on the recipient field)
3. Switch to jp-kanji IME
4. Press や (It is the third row of the third column)
5. Press ま (It is to the right of や, i.e. the second row of the second column)

At Step 5, adb logcat says
W/Built-in Keyboard( 1133): [JavaScript Error: "TypeError: cand is undefined" {file: "app://keyboard.gaiamobile.org/js/render.js" line: 544}]
And that IME basically becomes inoperable (further presses on the keys won't have any input effect, only highlights)
Attachment #8508773 - Flags: review?(jlu) → feedback+
Comment on attachment 8508773 [details] [review]
mozilla-b2g:master PR#25374

After some experimentation, it appears that my claim on comment 3 is incorrect. It's an issue that arises from Japanese IME's implementation for candidate suggestion and is observable in master if reproduced with correct timing.
Attachment #8508773 - Flags: feedback+ → review+
master: https://github.com/mozilla-b2g/gaia/commit/76d80e5b137dbe96dd41b132fc6a57deae8ea157
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.