1396953 - Flexible Space disappears from customize mode palette after adding it to the toolbar using the context menu

Status: VERIFIED FIXED

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

Description

[Erica Wright [:ewright]]

Attachment: flexible-space.mp4

STR: 
Go to customize mode 
right click on "flexible space" widget
select "add to toolbar"

AR:
when right clicking on the "flexible space" widget it disappears from the palette. and reappears in the toolbar.

ER:
the flexible space widget should stay in the palette and also appear in the toolbar.

Note: when dragging and dropping from the palette to the toolbar, the widget behaves as expected.

When the widget disappears from the palette, there is no way to get it back, neither from dragging and dropping from the toolbar, nor right clicking on the toolbar widget and selecting remove from toolbar.

Comment 1

[:Gijs (he/him)]

To fix this, we need to do a similar thing as this code does for dnd:

https://dxr.mozilla.org/mozilla-central/rev/34e2566a71f160eb3c5c3d92626453852e818f18/browser/components/customizableui/CustomizeMode.jsm#1943-1946

    // 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];
    }

in the addToToolbar method:

https://dxr.mozilla.org/mozilla-central/rev/34e2566a71f160eb3c5c3d92626453852e818f18/browser/components/customizableui/CustomizeMode.jsm#628-643

using `aNode.id` instead of `aDraggedItemId` in the code above. And to check that the item we're adding is in the palette, instead of `aOriginArea.id` (which isn't present) we can probably just check whether the node is in the customization palette using `aNode.closest()` with a selector (probably based on `kPaletteId`).

Comment 2

[Maya Messinger]

Think I can tackle this. I have a bit of experience with javascript, mostly editing a template code for a website, so I think I can do this within the next week. Thanks for being so helpful, Gijs!

Comment 3

[:Gijs (he/him)]

Thanks, Maya!

Comment 4

[Maya Messinger]

So I just started working on this tonight - been busy all week, I apologize, weekends are best for doing this - and I have an if statement but it doesn't seem to be working. I found the selector for the "kPaletteID", but think the issue might be with the second line.

    if (CustomizableUI.isSpecialWidget(aNode.id) && aNode.closest("#customization-palette")) {
      aNode.id = aNode.id.match(/^customizableui-special-(spring|spacer|separator)/)[1];
    }

I have a feeling the issue is with using .id.match, but it's possible that it's an incorrect use of .closest() part in the first line. I've tried a few variations of the if statement, and with aNode.parent, but that doesn't seem to be it. I know .isSpecialWidget(aNode.id) is valid (used elsewhere in the code).

I'll keep working on this through the week, but this is an update.

Comment 5

[Maya Messinger]

Actually, I think the issue is in the second line. I don't think setting id with = is the way to go.

Comment 6

[:Gijs (he/him)]

(In reply to Maya Messinger from comment #5)
> Actually, I think the issue is in the second line. I don't think setting id
> with = is the way to go.

Not on the node (aNode.id), no - but it should be enough in this place to have a local variable (aDraggedItemId) and assigning to that variable should be OK, I think? Of course then we need to pass that variable to CustomizableUI as the thing to move, when we get to that point. :-)

Comment 7

[:Gijs (he/him)]

Hey Maya, is there something else I can help with here? :-)

Comment 8

[Maya Messinger]

Hi Gijs, I'm super sorry, I've been pretty busy these past couple of weeks. I think I understand what you meant in your last comment, I've changed the lines to 

if (CustomizableUI.isSpecialWidget(aNode.id) && aNode.closest("#customization-palette")) {
      aDraggedItem = aNode.id.match(/^customizableui-special-(spring|spacer|separator)/)[1];
    }

This isn't the fix, but it seems that it's not supposed to be, completely? What's the next step, or did I misunderstand what you meant by using aDraggedItem? Thanks so much!

Comment 9

[:Gijs (he/him)]

(In reply to Maya Messinger from comment #8)
> Hi Gijs, I'm super sorry, I've been pretty busy these past couple of weeks.

No apologies necessary. I used to be a volunteer contributor when I was a student, I still remember what it's like. :-)

> I think I understand what you meant in your last comment, I've changed the
> lines to 
> 
> if (CustomizableUI.isSpecialWidget(aNode.id) &&
> aNode.closest("#customization-palette")) {
>       aDraggedItem =
> aNode.id.match(/^customizableui-special-(spring|spacer|separator)/)[1];
>     }
> 
> This isn't the fix, but it seems that it's not supposed to be, completely?
> What's the next step, or did I misunderstand what you meant by using
> aDraggedItem? Thanks so much!

Sorry, I should have checked more carefully when I made this suggestion.

The abridged version of what we need to do in this patch is as follows:

To add an item to the toolbar, both the drag/drop code and the context menu code calls CustomizableUI.addWidgetToArea().
Normally, the first argument to that method is the ID of the widget you want to add.
For special nodes that we're adding from the palette, we don't want to pass the ID of those special nodes, but the "type" (so "spring" or "separator", rather than "customizableui-special-spring5", as an example ID - the number at the end can vary, because the IDs have to be unique).
The CustomizableUI code will then deal with doing the right thing and creating a new node of the type you passed in.

Looking at:

https://dxr.mozilla.org/mozilla-central/rev/34e2566a71f160eb3c5c3d92626453852e818f18/browser/components/customizableui/CustomizeMode.jsm#628-643

it basically works based on a DOM node it gets passed in (aNode). It does some checks, and then tells CustomizableUI to add that node to the toolbar, with this line:

    CustomizableUI.addWidgetToArea(aNode.id, CustomizableUI.AREA_NAVBAR);

So in this case, we want to pass in a different string than the original node's ID. We can't just assign a different value to `aNode.id`, because the `id` property is readonly. So instead we need a separate variable that we can change. So we could do something like this:

    let widgetToAdd = aNode.id;
    if (CustomizableUI.isSpecialWidget(widgetToAdd) && aNode.closest("#customization-palette")) {
      // set widgetToAdd to the type of special widget we're adding:
      widgetToAdd.match(/^customizableui-special-(spring|spacer|separator)/)[1];
    }

and then pass `widgetToAdd` to `CustomizableUI.addWidgetToArea` instead of `aNode.id`.

Does that make sense?

Comment 10

[Maya Messinger]

Hi Gijs! Thanks so much for that explanation, it works perfectly! Unfortunately, I'm having an issue with version control. The library panel printing bug, that wasn't really a bug and that got reverted, kept a local copy on my computer with my change and got added to a commit. I then tried reverting that changeset and now there are 5 changeset reviews for you. The last one, is correct. Please disregard the others. But the last one is correct and works! Thanks so much!

Comment 11

[Maya Messinger]

Attachment: Bug 1396953 - fix flexible space widget disappearing when adding to toolbar

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

Comment 12

[Maya Messinger]

Attachment: Bug 1396953 fix flexible space widget disappearing from palette after toolbar add

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

Comment 13

[Maya Messinger]

Attachment: Backed out changeset 8beff0a7bc09

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

Comment 14

[Maya Messinger]

Attachment: Backed out changeset 4e4239384b2e

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

Comment 15

[Maya Messinger]

Attachment: Bug 1396953 Fix flexible space disappearing from palette after add to toolbar

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

Comment 16

[Maya Messinger]

Attachment: Bug 1396953 - fix flexible space widget disappearing when adding to toolbar

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

Comment 17

[Maya Messinger]

Attachment: Bug 1396953 fix flexible space widget disappearing from palette after toolbar add

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

Comment 18

[Maya Messinger]

Attachment: Backed out changeset 8beff0a7bc09

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

Comment 19

[Maya Messinger]

Attachment: Bug 1396953 - fix flexible space widget disappearing when adding to toolbar

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

Comment 20

[Maya Messinger]

Attachment: Bug 1396953 Fix flexible space disappearing from palette after add to toolbar

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

Comment 21

[Maya Messinger]

I can't seem to discard review requests without discarding all of them, so I'm leaving them all up and maybe you can approve only one? This request is the good one: https://reviewboard.mozilla.org/r/185390
I apologize if you were spammed with a ton of requests!

Comment 22

[Maya Messinger]

(In reply to Maya Messinger from comment #21)
> I can't seem to discard review requests without discarding all of them, so
> I'm leaving them all up and maybe you can approve only one? This request is
> the good one: https://reviewboard.mozilla.org/r/185398
> I apologize if you were spammed with a ton of requests!

Comment 23

[:Gijs (he/him)]

(In reply to Maya Messinger from comment #21)
> I can't seem to discard review requests without discarding all of them, so
> I'm leaving them all up and maybe you can approve only one? 

No worries. The last cset does look good. It also looks like all the diffs put together would also work. Unfortunately, if I review the last one right now, the review will just get lost if we move it to a single commit to try to autoland it. So let's try to go back to having 1 commit before I review it. I don't have admin powers or something to make this happen for you, but I can try suggesting ways of accomplishing it! :-)

To collapse all of these revisions into a single, correct commit, you should be able to do:


hg squash --exact --rev "4e4239384b2e::4cf76f399ed6" -m "Bug 1396953 - fix flexible space widget disappearing when adding to toolbar r?Gijs"


Then you'll see something like "5 changesets folded". You will need the "evolve" extension enabled for this to work. You can check before trying this by running "hg help squash". If it tells you there's "no such help topic" then you don't have the extension. 

If the "hg squash" command doesn't work (maybe you don't have the evolve extension installed, and don't want to bother trying to install it), you could also try this sequence of commands:

hg pull

hg rebase -s 4cf76f399ed6 -d central


Once you have this single new revision (either by folding the 5 other ones, or rebasing the top one onto the new central tip), you can push the resulting changeset to mozreview. Directly after either of the previous approaches, you should be able to use "hg up -r tip" to update to your new revision because it's the tip-most revision in your local repository (you could also use "hg wip" to check where you are, and what changeset you want), and then push to mozreview as usual.

Hopefully either of those helps? Let me know if you need more help.

Comment 24

[Maya Messinger]

Thanks for the troubleshooting! I got evolve and did the squash, but when I try to push, there's an "abort: error: getaddrinfo failed". Do you know what to do for this, or should I go for the rebase version? Thanks for all your help!

Comment 25

[:Gijs (he/him)]

(In reply to Maya Messinger from comment #24)
> Thanks for the troubleshooting! I got evolve and did the squash, but when I
> try to push, there's an "abort: error: getaddrinfo failed". Do you know what
> to do for this, or should I go for the rebase version? Thanks for all your
> help!

It sounds like the squash worked, so you don't need to use the rebase version.

The "getaddrinfo failed" error implies that hg couldn't connect to the server specified. What commandline are you using to push to mozreview? If you're pushing a changeset hash 'abcdef', something like this:

hg push -r abcdef review

should work, assuming the machine you're pushing from has a working internet connection and hg is set up correctly and so on. See http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#submitting-commits-for-review for more info.

Comment 26

[Maya Messinger]

Comment on attachment 8913989 [details]
Bug 1396953 - fix flexible space widget disappearing when adding to toolbar

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

Comment 27

[Maya Messinger]

Silly me, it was just an internet connection issue. Thank you so, so much for all of your help, Gijs!! I appreciate you walking me through these bugs a lot, and your friendliness and helpfulness! I think that this should finally be it, fixed. Thanks again!

Comment 28

[:Gijs (he/him)]

Comment on attachment 8913989 [details]
Bug 1396953 - fix flexible space widget disappearing when adding to toolbar

https://reviewboard.mozilla.org/r/185396/#review190320

Almost there! :-)

::: browser/components/customizableui/CustomizeMode.jsm:613
(Diff revision 2)
>      if (aNode.localName == "toolbarpaletteitem" && aNode.firstChild) {
>        aNode = aNode.firstChild;
>      }
>  
>      let panel = CustomizableUI.AREA_FIXED_OVERFLOW_PANEL;
> -    CustomizableUI.addWidgetToArea(aNode.id, panel);
> +    CustomizableUI.addWidgetToArea(widgetToAdd, panel);

`widgetToAdd` is only defined in the `addToToolbar` block, so this function currently throws an error on this line.

Because special nodes like flexible spaces can't be added to the panel, we can just leave this one as `aNode.id` for now. :-)

Comment 29

[Maya Messinger]

Comment on attachment 8913989 [details]
Bug 1396953 - fix flexible space widget disappearing when adding to toolbar

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

Comment 30

[Maya Messinger]

Oh, ****. Sorry, fixed! Commit amended, so should be here: https://reviewboard.mozilla.org/r/185388. Thanks!

Comment 31

[:Gijs (he/him)]

Comment on attachment 8913989 [details]
Bug 1396953 - fix flexible space widget disappearing when adding to toolbar

https://reviewboard.mozilla.org/r/185396/#review190322

Looks great, thanks!

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5cf95178314
fix flexible space widget disappearing when adding to toolbar r=Gijs

Comment 33

[Maya Messinger]

Haha, thank you for being so patient! Sorry this one took so much doing!

Comment 34

[:Gijs (he/him)]

(In reply to Maya Messinger from comment #33)
> Haha, thank you for being so patient! Sorry this one took so much doing!

No problem. The mess with the print (preview) button is kind of my own fault, sorry about that.

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Is that something that you will request an uplift for?

Comment 37

[:Gijs (he/him)]

Comment on attachment 8913989 [details]
Bug 1396953 - fix flexible space widget disappearing when adding to toolbar

(In reply to Sylvestre Ledru [:sylvestre] from comment #36)
> Is that something that you will request an uplift for?

Sure.

Approval Request Comment
[Feature/Bug causing the regression]: photon having the flexible spacer in customize mode
[User impact if declined]: it can disappear!
[Is this code covered by automated tests?]: generally yes; this specific part: no.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]:
1. open customize mode
2. right click the flexible space in the palette / list of tools
3. click 'add to toolbar'

ER: keep showing a flexible space in palette / list of tools, in addition to adding a new one at the end of the toolbar
AR (prior to patch): the one in the palette / list of tools disappears.

[List of other uplifts needed for the feature/fix]: nope
[Is the change risky?]: no
[Why is the change risky/not risky?]: very small change to the relevant bit of code. If it broke stuff completely it would have broken tests. The actual change is easy to test and verify
[String changes made/needed]: nope

Comment 38

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8913989 [details]
Bug 1396953 - fix flexible space widget disappearing when adding to toolbar

Polish photon, taking it.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bc4443239c37

Comment 40

[Vlad Bacia-Mociran [:VladB]]

User Agent:  	Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171008220130

I can reproduce the issue with Firefox Nightly v56.0a1, Build ID: 20170801100311 on Windows 8.1 x64.

This issue has been verified as fixed on latest Firefox Nightly Build ID: 20171008220130 on Windows 8.1 x64, Mac OS 10.12 and Ubuntu 14.04. Now, the flexible space can be added to browser toolbar and removed from toolbar from context menu without any issues.

This issue has been verified as fixed also on Firefox Beta 57.0b6.

Comment 41

[Ciprian Georgiu, Desktop QA]

Marking verified fixed per comment 40.