870901 - Make Edit control customizable

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

The Cut/Copy/Paste button in the panel needs to be able to be removed.

When it goes into a toolbar, it should be icons only, but still bound in the cut/copy/paste order.

The old individual cut, copy and paste buttons should be removed.

Comment 1

[:Gijs (he/him)]

I just removed the existing buttons; please remember to update the code here:

https://hg.mozilla.org/projects/ux/diff/a17f84ab819e/browser/base/content/browser.js

as it'll need to check for the existence of the new edit controls.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Re-add edit controls and make them customizable

This patch also re-adds the edit control styles that were removed from bug 873518.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Re-add edit controls and make them customizable

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Re-add edit controls and make them customizable

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Re-add edit controls and make them customizable

Unbitrotted patch.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Can't seem to copy and paste non-form page content:

http://screencast.com/t/uV5AHpu932

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike Conley (:mconley) from comment #6)
> Can't seem to copy and paste non-form page content:
> 
> http://screencast.com/t/uV5AHpu932

Is that a regression from this patch, or a separate bug that was already present?

Comment 8

[u428464]

(In reply to Jared Wein [:jaws] from comment #7)
> (In reply to Mike Conley (:mconley) from comment #6)
> > Can't seem to copy and paste non-form page content:
> > 
> > http://screencast.com/t/uV5AHpu932
> 
> Is that a regression from this patch, or a separate bug that was already
> present?

I've seen this for a little while now.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to Guillaume C. [:ge3k0s] from comment #8)
> (In reply to Jared Wein [:jaws] from comment #7)
> > (In reply to Mike Conley (:mconley) from comment #6)
> > > Can't seem to copy and paste non-form page content:
> > > 
> > > http://screencast.com/t/uV5AHpu932
> > 
> > Is that a regression from this patch, or a separate bug that was already
> > present?
> 
> I've seen this for a little while now.

Ok, if that's the case, we don't block on it, but we should file a follow-up bug. I'll do that now.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Filed bug 878106.

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Re-add edit controls and make them customizable

I added the noautoclose=true attribute to the panel in bug 870897

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Re-add edit controls and make them customizable

Unbitsnotting

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 756833 [details] [diff] [review]
Re-add edit controls and make them customizable

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

Thanks!

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +383,5 @@
>        return node;
>      }
> +  }, {
> +    id: "edit-controls",
> +    name: "Edit Controls",

If this lands first, please update your localization strings patch to account for this widget (and the zoom widget, I guess)

@@ +415,5 @@
> +        label: "Paste",
> +        tooltiptext: "Paste"
> +      }];
> +
> +      function setAttributes(aNode, aAttrs) {

See my comment in the Zoom control review about making this available to all widgets in CustomizableWidget.jsm.

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Re-add edit controls and make them customizable

Carrying over r=mconley

Comment 15

[:Gijs (he/him)]

Pushed: http://hg.mozilla.org/projects/ux/rev/ea38f867cc07

Note that this didn't touch the hunk I mentioned in comment #1, and possibly should have.

Mike (Conley), can we make that code just check for one of the buttons, or is the overflow stuff in the navbar going to make that not always work and should we check for all 3? Mike (de Boer) also told me he didn't see any issues enabling/disabling the buttons as-is. Just needinfo'ing you here to see what we do about this, if anything.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #15)
> Pushed: http://hg.mozilla.org/projects/ux/rev/ea38f867cc07
> 
> Note that this didn't touch the hunk I mentioned in comment #1, and possibly
> should have.
> 

Good point. I'll file a follow-up.

> Mike (Conley), can we make that code just check for one of the buttons, or
> is the overflow stuff in the navbar going to make that not always work and
> should we check for all 3?

I think we should be fine just checking for the root item - the cut, copy and paste buttons will always be contained in it, overflowed or not.

> Mike (de Boer) also told me he didn't see any issues enabling/disabling
> the buttons as-is. Just needinfo'ing you here to see what we do about this,
> if anything.

Yep, should still be fine.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Backed out due to possibly introducing leaks:

https://hg.mozilla.org/projects/ux/rev/29f42f35c30e

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Re-add edit controls and make them customizable

addressed memory leak

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Re-add edit controls and make them customizable

Unbitrotted patch. Carrying over r=mconley

Comment 20

[:Gijs (he/him)]

Pushed again: https://hg.mozilla.org/projects/ux/rev/eef2d494d353

Comment 21

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/eef2d494d353