[jsplugins] Implement PPB_KeyboardInputEvent_Create

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ywu, Assigned: ywu)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 years ago
We need to implement PPB_KeyboardInputEvent_Create for "backspace" and "escape".

Comment 1

2 years ago
(In reply to Ya-Chieh Wu from comment #0)
> We need to implement PPB_KeyboardInputEvent_Create for "backspace" and
> "escape".

... for erasing one character when hitting backspace key.
(Assignee)

Comment 2

2 years ago
(In reply to Evelyn Hung [:evelyn] from comment #1)
> (In reply to Ya-Chieh Wu from comment #0)
> > We need to implement PPB_KeyboardInputEvent_Create for "backspace" and
> > "escape".
> 
> ... for erasing one character when hitting backspace key.

Erasing one character when hitting backspace key is not the only case. This API will be called when either "backspace" or "escape" are hitting, no matter you are typing or not.
(Assignee)

Comment 3

2 years ago
Created attachment 8827376 [details] [diff] [review]
0001-Implement-PPB_KeyboardInputEvent_Create.patch

By applying this patch, you can be able to erase one character in our pdf form. Now, you can open pdf form on a second tab, which doesn't have previous page[1], to test. 

Hitting "ESC" also gets called of this API but for "ESC", we need to further fix Bug 1331298.   

[1]Bug 1273472
(Assignee)

Comment 4

2 years ago
Created attachment 8830588 [details] [diff] [review]
Implement PPB_KeyboardInputEvent_Create

Hi Peter, 

Could you help with this patch? PPB_KeyboardInputEvent_Create is called when "ESC" or "Backspace" are pressed. 

(1) "Backspace"
Since hitting "Backspace" will cause us go back to previous page[1], we can open a second tab, which doesn't have a previous page, to test if hitting "Backspace" can erase characters. 

(2) "ESC"
"ESC" is parsed to json escape format which we haven't fully support yet. Thus, for "ESC" to work, we need to fix Bug 1331298.
Attachment #8827376 - Attachment is obsolete: true
Attachment #8830588 - Flags: review?(peterv)
Comment on attachment 8830588 [details] [diff] [review]
Implement PPB_KeyboardInputEvent_Create

Review of attachment 8830588 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/mortar/host/common/ppapi-runtime.jsm
@@ +3066,5 @@
> +      if (PP_VarType[json.character_text.type] == PP_VarType.PP_VARTYPE_STRING) {
> +        charCode = String_PP_Var.getAsJSValue(json.character_text).charCodeAt();
> +      }
> +      let keyboardEventInit = {
> +        modifiers: json.modifiers,

This is wrong, there is no modifiers property in EventModifierInit. You need to do something like: |altkey: PP_InputEvent_Modifier.PP_INPUTEVENT_MODIFIER_ALTKEY & json.modifiers,|, etc. Also, shouldn't you set code, according to the PPAPI documentation it "represents the DOM3 |code| string that corresponds to the physical key being pressed", so it seems like passing it through makes sense?
Attachment #8830588 - Flags: review?(peterv) → review-
Also, I think we should ignore charCode if we can. I wonder if that makes the ESC key work without the patch from bug 1331298?
(Assignee)

Comment 7

2 years ago
(In reply to Peter Van der Beken [:peterv] from comment #5)
> Also, shouldn't you set code, according to the PPAPI documentation it
> "represents the DOM3 |code| string that corresponds to the physical key
> being pressed", so it seems like passing it through makes sense?

I ignore "code" because I can't find an useful "code value" passing from plugin to gecko. At this point, I only can see "Backspace" & "ESC" use this PPB_KeyboardInputEvent:Create API.  And every time either of these key is hitting, the "code" passed to us is | code":{"type":"PP_VARTYPE_UNDEFINED","padding":0,"value":{"as_bool":"PP_FALSE","as_int":0,"as_double":0,"as_id":0}} | which doesn't really contain any information.
(Assignee)

Comment 8

2 years ago
(In reply to Peter Van der Beken [:peterv] from comment #6)
> Also, I think we should ignore charCode if we can. I wonder if that makes
> the ESC key work without the patch from bug 1331298?

After plugin calls PPB_KeyboardInputEvent:Create, it immediately calls PPB_KeyboardInputEvent:GetCharacterText where it needs the value of |charCode| to return. I don't know if we are able to ignore charCode here.
(Assignee)

Comment 9

2 years ago
Created attachment 8837036 [details] [diff] [review]
Implement-PPB_KeyboardInputEvent_Create.patch

Hi peter,

I addressed the |modifiers| issue which mentioned in Comment 5. And for skipping to pass |code| into plugin and needing to use |charCode|, I put the reasons in Comment 7 and Comment 8. 

thanks.
Attachment #8830588 - Attachment is obsolete: true
Attachment #8837036 - Flags: review?(peterv)
(Assignee)

Comment 10

2 years ago
Created attachment 8839069 [details] [diff] [review]
Bug-1325896-Implement-PPB_KeyboardInputEvent_Create.patch

Using KeyboardEvent.location to set the location information.
Attachment #8837036 - Attachment is obsolete: true
Attachment #8837036 - Flags: review?(peterv)
Attachment #8839069 - Flags: review?(peterv)
Comment on attachment 8839069 [details] [diff] [review]
Bug-1325896-Implement-PPB_KeyboardInputEvent_Create.patch

Review of attachment 8839069 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/mortar/host/common/ppapi-runtime.jsm
@@ +3064,5 @@
> +      let instance = this.instances[json.instance];
> +      let charCode = 0;
> +      if (PP_VarType[json.character_text.type] ==
> +          PP_VarType.PP_VARTYPE_STRING) {
> +        charCode = String_PP_Var.getAsJSValue(json.character_text).charCodeAt();

Please pass in 0 to charCodeAt.
Attachment #8839069 - Flags: review?(peterv) → review+
(Assignee)

Comment 12

2 years ago
Created attachment 8844289 [details] [diff] [review]
0001-Bug-1325896-Implement-PPB_KeyboardInputEvent_Create..patch

Result of try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e914cc63d160b8aed08832e1f28e31369808d4a
Attachment #8839069 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e578d0aa1440
Implement PPB_KeyboardInputEvent_Create. r=peterv
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e578d0aa1440
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.