Add customizable flexible space item in customize mode

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
3 months ago
13 days 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])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months 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+

Updated

3 months ago
Priority: -- → P2
QA Contact: gwimberly
(Assignee)

Comment 1

3 months 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

3 months 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

3 months 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

a month ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Priority: P2 → P1
(Assignee)

Updated

a month ago
See Also: → bug 1379821
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8885012 - Flags: review?(mconley) → review?(dtownsend)
(Assignee)

Comment 5

a month 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

a month 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

a month 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

a month 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

a month 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

a month 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
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
Flags: needinfo?(gijskruitbosch+bugs)

Comment 15

a month 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

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0e4a948fbae2
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Verified on Windows, Mac, and Linux.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
Blocks: 1387512
(Assignee)

Updated

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