1366813 - Add customizable flexible space item in customize mode

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[:Gijs (he/him)]

Per https://mozilla.invisionapp.com/share/BXBFSCU6P#/screens/229252091_Customization

- there should be a flexible spacer item in the customization palette
- it shouldn't disappear if you drag it
- you can only add flexible spacers to toolbars, not panels
-- flexible spacers are display: none when in the dynamic portion of the overflow panel, and we hardcode their min-width as the required space to re-introduce them in the overflown toolbar (this should be handle-able within the overflowable toolbar code for moving items in/out in CustomizableUI.jsm)
- flexible spacers are visualized in the toolbars with dotted lines when in customize mode
- flexible spacers get a flex of X
- flexible spacers get a min-width of Y

Bryan/Stephen, can you provide details for X and Y?

Comment 1

[:Gijs (he/him)]

(In reply to :Gijs from comment #0)
> - flexible spacers get a flex of X
> - flexible spacers get a min-width of Y
> 
> Bryan/Stephen, can you provide details for X and Y?

+ni

Comment 2

[Aaron Benson]

- flexible spacers get a flex of X
I assume this refers to how wide the flexible spacer can get? If so, their width is a function of how wide the toolbar is since the flexible spacer fills in as much space between objects as possible. 

- flexible spacers get a min-width of Y
40px (width of a standard toolbar item)

Comment 3

[Aaron Benson]

I wasn't picking up that this was referring to flex css property (smile).

The flexible spacers flex value should be 20% of what the URL bar is, though we'll likely want to fine tune this.

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1366813 - add a flexible space item in customize mode in photon,

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

Comment 5

[:Gijs (he/him)]

Comment on attachment 8885012 [details]
Bug 1366813 - add a flexible space item in customize mode in photon,

https://reviewboard.mozilla.org/r/155834/#review160992

::: browser/base/content/browser.css:271
(Diff revision 1)
> +toolbarpaletteitem {
> +  -moz-window-dragging: no-drag;
> +}

On OS X, without this, if you put a flexible space in the tabstrip you can't drag it out (in customize mode) because the window gets dragged instead.

::: browser/components/customizableui/CustomizableUI.jsm:3810
(Diff revision 1)
>    },
> +
> +  /**
> +   * Create an instance of a spring, spacer or separator.
> +   * @param aId         the type of special widget (spring, spacer or separator)
> +   * @param aDocumentId the document in which to create it.

Self-nit: aDocument, of course.

::: browser/components/customizableui/CustomizeMode.jsm:885
(Diff revision 1)
>        this.visiblePalette.hidden = true;
>        let paletteChild = this.visiblePalette.firstChild;
>        let nextChild;
>        while (paletteChild) {
>          nextChild = paletteChild.nextElementSibling;
> -        let provider = CustomizableUI.getWidget(paletteChild.id).provider;
> +        let itemId = paletteChild.firstChild.id;

So, this was broken. It's checking the widget based on the toolbarpaletteitem's (ie the wrapper's) id, which is always going to return an unknown widget wrapper, which defaults to XUL (for add-on backward compat).

So in practice, anything in the palette was being stowed.

Trying to 'fix' this so we use the right ID broke two things:
1) widget API items got duplicated the next time you entered customize mode. From this, I deduce that adding the widget API items to the stowed palette is just the right thing to do (while destroying instances isn't an option, as the comment notes), even if this code originally intended something else.
2) it still didn't remove the extra 'special' (ie space, flexible space, separator) items because of bug 1379821 (filed separately).

As a result, I just changed this to check specifically for special widgets and outright remove them (in practice, this is only the special widget we added for photon), and I stow everything else. I moved the comment and rephrased slightly so it continued to make sense.

::: browser/components/customizableui/CustomizeMode.jsm:1776
(Diff revision 1)
> +    if (CustomizableUI.isSpecialWidget(draggedItem.id)) {
> +      let panelHolder = item.ownerDocument.getElementById("customization-panelHolder");
> +      panelHolder.setAttribute("dragging-special", "true");
> +    }

This and removing it is to ensure that the cursor indicates the user can't drop these items in the panel.

::: browser/components/customizableui/CustomizeMode.jsm:2075
(Diff revision 1)
>      }
>      let position = placement ? placement.position : null;
>  
> +    // Force creating a new spacer/spring/separator if dragging from the palette
> +    if (CustomizableUI.isSpecialWidget(aDraggedItemId) && aOriginArea.id == kPaletteId) {
> +      aDraggedItemId = aDraggedItemId.match(/^customizableui-special-(spring|spacer|separator)/)[1];

If we call the move/add API with just "spring" or "spacer" or "separator", CustomizableUI itself takes care of creating a new instance specific to the new location.

This wouldn't be right if the user is moving an item in the palette, but in that case we will have bailed out earlier (see context further up).

::: npm-shrinkwrap.json:542
(Diff revision 1)
>        }
>      },
>      "glob": {
>        "version": "7.1.2",
>        "resolved": "https://registry.npmjs.org/glob/-/glob-7.1.2.tgz",
> -      "integrity": "sha512-MJTUg1kjuLeQCJ+ccE4Vpa6kKVXkPYJ2mOCQyUuKLcLQsdrMCpBPUi8qVE6+YuaJkozeA9NusTAw3hLr8Xe5EQ==",
> +      "integrity": "sha1-wZyd+aAocC1nhhI4SmVSQExjbRU=",

Uh, I have no idea why this is in the patch. :-\

Comment 6

[:Gijs (he/him)]

Comment on attachment 8885012 [details]
Bug 1366813 - add a flexible space item in customize mode in photon,

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

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8885012 [details]
Bug 1366813 - add a flexible space item in customize mode in photon,

https://reviewboard.mozilla.org/r/155834/#review161000

The code seems ok but there are a few issues with the current patch:

(In reply to :Gijs from comment #0)
> - you can only add flexible spacers to toolbars, not panels

The drag-drop UI still suggests that you can drop it on the overflow panel with this patch applied.

> - flexible spacers are visualized in the toolbars with dotted lines when in
> customize mode

This isn't the case with the current patch.

The current display on Windows isn't very well lined up with other items.

Comment 8

[:Gijs (he/him)]

(In reply to Dave Townsend [:mossop] from comment #7)
> Comment on attachment 8885012 [details]
> Bug 1366813 - add a flexible space item in customize mode in photon,
> 
> https://reviewboard.mozilla.org/r/155834/#review161000
> 
> The code seems ok but there are a few issues with the current patch:
> 
> (In reply to :Gijs from comment #0)
> > - you can only add flexible spacers to toolbars, not panels
> 
> The drag-drop UI still suggests that you can drop it on the overflow panel
> with this patch applied.

Hm, I wrote this on OS X and assumed this was a side-effect of bug 1320065, but you know what they say about assumptions.

Anyway, this made me realize that there was pre-existing code for this that worked if you tried to move separators/spacers into the panel *from a toolbar*, but it didn't work for items in the palette (that is, for which getPlacementOfWidget returned null). Fixed that, which means less useless code added, yay. Note that this will mean the dragging icon is correct on Windows and Linux. I am not aware of a way of making it correct on OS X (per earlier bug).

> > - flexible spacers are visualized in the toolbars with dotted lines when in
> > customize mode
> 
> This isn't the case with the current patch.

I said dotted in comment #0, I don't think that's correct, looking at https://mozilla.invisionapp.com/share/BXBFSCU6P#/screens/229252092 . So I think the current styling of that border/outline is OK.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8885012 [details]
Bug 1366813 - add a flexible space item in customize mode in photon,

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

Comment 10

[:Gijs (he/him)]

Comment on attachment 8885012 [details]
Bug 1366813 - add a flexible space item in customize mode in photon,

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

Comment 11

[:Gijs (he/him)]

So it seems the cause of the misalignment and the need for negative margins etc. is to do with how the code detects the baseline of the toolbarspring item (which has no text, unlike every other item in customize mode). Instead of trying to compensate for this, I switched the vertical-align on these items to top, adjusted the height of the spacer to match (and its outline-offset so it isn't really tall compared to how wide it is), and adjusted the size of the search container when it's in the palette so it matches the height of the other items (37px, ie 16px + 12 + 9px padding). This seems to work. The combined items (edit/zoom controls) have looked a bit off for a while. I'm changing their styling substantially in bug 1354086 so I'll make any remaining tweaks for those there. This looks OK for me on both Windows and OS X.

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8885012 [details]
Bug 1366813 - add a flexible space item in customize mode in photon,

https://reviewboard.mozilla.org/r/155834/#review161348

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/051284a3564e
add a flexible space item in customize mode in photon, r=mossop

Comment 14

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

A bunch of autophone tests started frequently failing when this landed. Backed out in https://hg.mozilla.org/integration/autoland/rev/e336727983aecef65a45dc24901cf1f6be20133c to see if the failures go away.

https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=d816a7f555299b4f66b9690dbdd59c2a7afa9440&noautoclassify&filter-searchStr=autop&group_state=expanded&tochange=e336727983aecef65a45dc24901cf1f6be20133c

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e4a948fbae2
add a flexible space item in customize mode in photon, r=mossop

Comment 16

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

Relanded because the failures were still happening (and now I see they're also happening on inbound).

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e4a948fbae2

Comment 18

[u554753]

Verified on Windows, Mac, and Linux.