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

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect, P1)

Description

[Louis Chang [:louis]]

Implement the function of select all text <Ctrl+a>

Comment 1

[Louis Chang [:louis]]

Transfer bug to Luke since it should be handled at UI.

Comment 2

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

Review commit: https://reviewboard.mozilla.org/r/119014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119014/

Comment 3

[KM Lee [:rexboy] (inactivate after Sep. 22)]

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.

Comment 4

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

Nice implementation. Thanks.

Comment 5

[Evelyn Hung]

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.

Comment 6

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119014/diff/1-2/

Comment 7

[KM Lee [:rexboy] (inactivate after Sep. 22)]

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?

Comment 8

[Louis Chang [:louis]]

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.

Comment 9

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

Thanks Louis!

Let's proceed the review process then.

Comment 10

[Evelyn Hung]

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?

Comment 11

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119014/diff/2-3/

Comment 12

[KM Lee [:rexboy] (inactivate after Sep. 22)]

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?

Comment 13

[Luke Chang [:lchang] (inactive)]

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.

Comment 14

[Evelyn Hung]

Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

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

LGTM.

Comment 15

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8845867 [details]
Bug 1287012 - [mortar][PDF] Enable select all feature. f=lchang,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119014/diff/3-4/

Comment 16

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Thanks!
Let's check it in.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e99a3ae690a
[mortar][PDF] Enable select all feature. f=lchang, r=evelyn

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e99a3ae690a