Closed Bug 1287012 Opened 6 years ago Closed 5 years ago

[jsplugins][UI] Implement the function of select all text <Ctrl+a>

Categories

(Firefox :: PDF Viewer, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: lochang, Assigned: rexboy)

References

Details

(Whiteboard: [pdfium-viewer-mvpscope])

Attachments

(1 file)

Implement the function of select all text <Ctrl+a>
Assignee: nobody → lochang
Transfer bug to Luke since it should be handled at UI.
Assignee: lochang → lchang
Blocks: 1299395
No longer blocks: 1264551
Whiteboard: [pdfium-viewer-mvpscope]
Summary: [jsplugins] Implement the function of select all text <Ctrl+a> → [jsplugins][UI] Implement the function of select all text <Ctrl+a>
Group: mozilla-employee-confidential
Assignee: lchang → rexboy
Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

Implemented by just inserting controller to handle global command cmd_selectAll.

I didn't find a way to import commandset/command components into html; If that's possible the patch may have chance to be further simpler.

selectAll is relatively slow on PDFium (responded at about 1 seconds on my notebook with debug build). This may need to be noted when we check overall performance later.
Attachment #8845867 - Flags: feedback?(lchang)
Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

Nice implementation. Thanks.
Attachment #8845867 - Flags: feedback?(lchang) → feedback+
Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

https://reviewboard.mozilla.org/r/119014/#review121360

So far the patch looks good, but we need to test more for all entry points of select-all functionality to ensure we catch every case and deal with them correctly.

::: commit-message-35398:2
(Diff revision 1)
> +Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang, r=evelyn
> +

Please add more messages here. It's better to mention that we use XUL API and listen to command, instead of key event. Thanks.

::: browser/extensions/mortar/host/pdf/chrome/js/command-controller.js:19
(Diff revision 1)
> +  supportsCommand(cmd) {
> +    return cmd == "cmd_selectAll";
> +  }
> +
> +  isCommandEnabled(cmd) {
> +    return cmd == "cmd_selectAll";

Is it possible that in some case we don't want to perform select-all? I'm thinking when the user is typing on a form, I'm not sure the correct behavior though.
Attachment #8845867 - Flags: review?(ehung)
Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

Seems we need to handle command event from sandbox side and send it into the frame to ensure events from menu bar are caught correctly since command events from there are sent to the topmost iframe only.

Actually I don't know if we agree we can do that in sandbox. Louis can you give me some feedback first?
Attachment #8845867 - Flags: review?(ehung) → feedback?(lochang)
Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

I think we are good to listen the events in sandbox side.
Attachment #8845867 - Flags: feedback?(lochang) → feedback+
Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

Thanks Louis!

Let's proceed the review process then.
Attachment #8845867 - Flags: review?(ehung)
Attachment #8845867 - Flags: feedback?(lchang)
Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

https://reviewboard.mozilla.org/r/119014/#review121822

Overall looks good but I'd like to clarify the usage of `_DISABLED_COMMAND`. I feel the design should match our usage of XUL API. If we treat "support" and "enable" the same, then I don't see we need two arrays for the commands record. Thanks.

::: browser/extensions/mortar/host/pdf/chrome/js/viewport.js:743
(Diff revision 2)
>        x: this._viewportController.scrollLeft,
>        y: this._viewportController.scrollTop
>      };
>    }
>  
> +  selectAll() {

Will we call this method somewhere else? I'd prefer to move this into ` _handleCommand()` since it's simple and we've had `cmd_copy` do the same thing there.

::: browser/extensions/mortar/host/pdf/ppapi-content-sandbox.js:193
(Diff revision 2)
> +// triggered from menu bar targets only the outmost iframe (which is sandbox
> +// itself) so we need to propagate it manually into the plugin's iframe.
> +class CommandController {
> +  constructor() {
> +    this._SUPPORTED_COMMAND = ['cmd_copy', 'cmd_selectAll'];
> +    this._DISABLED_COMMAND = [''];

What's the purpose of this `_DISABLED_COMMAND` design? Are we expecting that someday we need to explicitly saying one command is not allowed? 

I feel the design of `supportsCommand` and `isCommandEnabled` of this XUL API isn't very clear to me. They sound like we could have one command supported but not enabled, what does that mean? What's different from saying it's not supported?
Attachment #8845867 - Flags: review?(ehung)
Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

Sorry for the late reply. Changes are addressed and tested.
May you take a look again?
Attachment #8845867 - Flags: feedback?(lchang)
Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

https://reviewboard.mozilla.org/r/119014/#review122408

::: browser/extensions/mortar/host/pdf/ppapi-content-sandbox.js:192
(Diff revision 3)
> +// The main reason we need to transfer it from sandbox side is that commands
> +// triggered from menu bar targets only the outmost iframe (which is sandbox
> +// itself) so we need to propagate it manually into the plugin's iframe.
> +class CommandController {
> +  constructor() {
> +    this._SUPPORTED_COMMANDS = ['cmd_copy', 'cmd_selectAll'];

nit: We don't use underscore prefix on constants.
Attachment #8845867 - Flags: feedback?(lchang) → feedback+
Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

https://reviewboard.mozilla.org/r/119014/#review122444

LGTM.
Attachment #8845867 - Flags: review?(ehung) → review+
Thanks!
Let's check it in.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e99a3ae690a
[mortar][PDF] Enable select all feature. f=lchang, r=evelyn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e99a3ae690a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.