All users were logged out of Bugzilla on October 13th, 2018

basicLayoutKey is not displayed correctly when it is defined by keyboard

RESOLVED FIXED

Status

--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: seinlin, Assigned: seinlin)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 801986 [details]
Arabic keyboard defines a basicLayoutKey, but alternateLayout displays it as "ABC".

The design of keyboard gives the author the ability to change the basic layout key contents, but it is not displayed correctly when it is defined by keyboard.

Attached file:

Arabic keyboard defines basicLayoutKey,

Expect result: In alternateLayout the basicLayoutKey should displays the content defined by Arabic keyboard 

Actual result: In alternateLayout the basicLayoutKey displays "ABC".
(Assignee)

Comment 1

5 years ago
Created attachment 801999 [details] [diff] [review]
bug-914459-pull-request.html

When keyboard defined basicLayoutKey, it should be used in alternateLayout and symbolLayout.
Attachment #801999 - Flags: review?(lchang)
(Assignee)

Comment 2

5 years ago
Created attachment 802025 [details]
bug-914459-pull-request.html

Re-attach the file in correct format.
Attachment #801999 - Attachment is obsolete: true
Attachment #801999 - Flags: review?(lchang)
Attachment #802025 - Flags: review?(lchang)
Comment on attachment 802025 [details]
bug-914459-pull-request.html

It looks good to me. :)
However, I think it should be reviewed by the keyboard owner.

@Rudy, could you please review this patch, thanks.
Attachment #802025 - Flags: review?(rlu)
Attachment #802025 - Flags: review?(lchang)
Attachment #802025 - Flags: feedback+
(Assignee)

Comment 4

5 years ago
Created attachment 802143 [details]
bug-914459-pull-request-2.html

Correct the format and send pull request again.
Attachment #802025 - Attachment is obsolete: true
Attachment #802025 - Flags: review?(rlu)
(Assignee)

Updated

5 years ago
Attachment #802143 - Flags: review?(rlu)
(Assignee)

Comment 5

5 years ago
Created attachment 802817 [details]
bug-914459-pull-request-3.html

When a keyboard defines basicLayoutKey but no alternateLayout, it will use the base alternateLayout + basicLayoutKey. Need to clean the basicLayoutKey after it is assigned, otherwise the basicLayoutKey will be used by other keyboard which do not define an alternateLayout.
Attachment #802143 - Attachment is obsolete: true
Attachment #802143 - Flags: review?(rlu)
Attachment #802817 - Flags: review?(rlu)
Comment on attachment 802817 [details]
bug-914459-pull-request-3.html

Hi Seinlin, 

I leave some comment on the pull reqeust, we might need to put the logic to replace basicLayoutKey in one place if possible.

Please help take a look and set the review flag back when you have an update.
Thanks.
Attachment #802817 - Flags: review?(rlu)
Change the assignee, since Seinlin has a WIP patch.

Hi Seinlin,

BTW, you may update the code in the same pull request if you just pushed to the same branch.
You don't have to create another pull request. :)
Assignee: nobody → kli
(Assignee)

Comment 8

5 years ago
(In reply to Rudy Lu [:rudyl] from comment #7)
> Change the assignee, since Seinlin has a WIP patch.
> 
> Hi Seinlin,
> 
> BTW, you may update the code in the same pull request if you just pushed to
> the same branch.
> You don't have to create another pull request. :)

Hi Rudy,

Thanks! I update change to the same pull request. Please have a look.
(Assignee)

Comment 9

5 years ago
Comment on attachment 801986 [details]
Arabic keyboard defines a basicLayoutKey, but alternateLayout displays it as "ABC".

test
Attachment #801986 - Flags: feedback?(seinlin.maung)

Comment 10

5 years ago
feedback
(Assignee)

Updated

5 years ago
Attachment #801986 - Flags: feedback?(seinlin.maung)
Comment on attachment 802817 [details]
bug-914459-pull-request-3.html

Hi Jan,

Not sure if you are interested in reviewing this patch first?
Attachment #802817 - Flags: review?(janjongboom)
Comment on attachment 802817 [details]
bug-914459-pull-request-3.html

r=me. The code works, but looks very strange if you see it in the codebase. Please add a comment that it's not possible to use |layout| variable here because it holds the alternative layout that doesn't have information about the alt key to be used; and that we need this info from the main layout object.

Please needinfo me if you added the comment and I'll land the patch :-) Thanks for your help!
Attachment #802817 - Flags: review?(janjongboom) → review+
(Assignee)

Comment 13

5 years ago
Thanks, I added a comment and pull request is updated!
Flags: needinfo?(janjongboom)
Hi, thanks a lot for a working on this. The patch has been landed in https://github.com/mozilla-b2g/gaia/commit/042b3f3bf882f2b3cb91207d54927b2490d65b9e.
Flags: needinfo?(janjongboom)
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.