Add customizable flexible space item in customize mode

VERIFIED FIXED in Firefox 56

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Depends on 1 bug, Blocks 3 bugs)

53 Branch
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [photon-structure])

Attachments

(1 attachment)

Assignee

Description

2 years ago
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?
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Assignee

Comment 1

2 years ago
(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
Flags: needinfo?(shorlander)
Flags: needinfo?(bbell)

Comment 2

2 years ago
- 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)
Flags: needinfo?(shorlander)
Flags: needinfo?(bbell)

Comment 3

2 years ago
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.
Assignee

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Priority: P2 → P1
Assignee

Updated

2 years ago
See Also: → 1379821
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8885012 - Flags: review?(mconley) → review?(dtownsend)
Assignee

Comment 5

2 years ago
mozreview-review
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 hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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.
Attachment #8885012 - Flags: review?(dtownsend)
Assignee

Comment 8

2 years ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 11

2 years ago
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

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

https://reviewboard.mozilla.org/r/155834/#review161348
Attachment #8885012 - Flags: review?(dtownsend) → review+

Comment 13

2 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/051284a3564e
add a flexible space item in customize mode in photon, r=mossop

Comment 15

2 years ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0e4a948fbae2
add a flexible space item in customize mode in photon, r=mossop
Relanded because the failures were still happening (and now I see they're also happening on inbound).
Flags: needinfo?(gijskruitbosch+bugs)

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0e4a948fbae2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Comment 18

2 years ago
Verified on Windows, Mac, and Linux.
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

2 years ago
Blocks: 1387512
Assignee

Updated

2 years ago
Depends on: 1389577
You need to log in before you can comment on or make changes to this bug.