Closed
Bug 1287012
Opened 9 years ago
Closed 8 years ago
[jsplugins][UI] Implement the function of select all text <Ctrl+a>
Categories
(Firefox :: PDF Viewer, defect, P1)
Firefox
PDF Viewer
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>
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → lochang
Reporter | ||
Comment 1•9 years ago
|
||
Transfer bug to Luke since it should be handled at UI.
Assignee: lochang → lchang
Updated•9 years ago
|
Priority: -- → P1
Whiteboard: [pdfium-viewer-mvpscope]
Updated•9 years ago
|
Summary: [jsplugins] Implement the function of select all text <Ctrl+a> → [jsplugins][UI] Implement the function of select all text <Ctrl+a>
Updated•8 years ago
|
Group: mozilla-employee-confidential
Assignee | ||
Updated•8 years ago
|
Assignee: lchang → rexboy
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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 5•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 7•8 years ago
|
||
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)
Reporter | ||
Comment 8•8 years ago
|
||
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+
Assignee | ||
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
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 13•8 years ago
|
||
mozreview-review |
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.
Updated•8 years ago
|
Attachment #8845867 -
Flags: feedback?(lchang) → feedback+
Comment 14•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 16•8 years ago
|
||
Thanks!
Let's check it in.
Comment 17•8 years ago
|
||
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
Comment 18•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 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.
Description
•