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

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: ewright, Assigned: maya.messinger, Mentored)

Tracking

57 Branch
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 attachments, 9 obsolete attachments)

1.98 MB, video/mp4
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Reporter

Description

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

Updated

2 years ago
Summary: Flexible Space disappears after adding to/removing from toolbar → Flexible Space disappears from customize mode palette after adding it to the toolbar using the context menu
Whiteboard: [photon-structure][triage]
Flags: qe-verify?
Priority: -- → P4
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly

Comment 1

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

Comment 2

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

Updated

2 years ago
See Also: → 1401152

Comment 3

2 years ago
Thanks, Maya!
Assignee: nobody → maya.messinger
Status: NEW → ASSIGNED
Priority: P4 → P1
Assignee

Comment 4

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

Comment 5

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

Comment 6

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

2 years ago
Hey Maya, is there something else I can help with here? :-)
Mentor: gijskruitbosch+bugs
Flags: needinfo?(maya.messinger)
Assignee

Comment 8

2 years ago
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!
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Updated

2 years ago
Flags: needinfo?(maya.messinger)

Comment 9

2 years ago
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(maya.messinger)
Assignee

Comment 10

2 years ago
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!
Flags: needinfo?(maya.messinger)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8913980 - Attachment is obsolete: true
Attachment #8913980 - Flags: review?(gijskruitbosch+bugs)
Assignee

Updated

2 years ago
Attachment #8913981 - Attachment is obsolete: true
Attachment #8913981 - Flags: review?(gijskruitbosch+bugs)
Assignee

Updated

2 years ago
Attachment #8913982 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8913983 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8913984 - Attachment is obsolete: true
Attachment #8913984 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 21

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

Comment 22

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

2 years ago
(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.
Flags: needinfo?(maya.messinger)
Assignee

Comment 24

2 years ago
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!
Flags: needinfo?(maya.messinger) → needinfo?(gijskruitbosch+bugs)

Comment 25

2 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(maya.messinger)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8913986 - Attachment is obsolete: true
Attachment #8913986 - Flags: review?(gijskruitbosch+bugs)
Assignee

Updated

2 years ago
Attachment #8913987 - Attachment is obsolete: true
Attachment #8913987 - Flags: review?(gijskruitbosch+bugs)
Assignee

Updated

2 years ago
Attachment #8913988 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8913990 - Attachment is obsolete: true
Attachment #8913990 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 27

2 years ago
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!
Flags: needinfo?(maya.messinger)

Comment 28

2 years ago
mozreview-review
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. :-)
Attachment #8913989 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Assignee

Comment 30

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

Comment 31

2 years ago
mozreview-review
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!
Attachment #8913989 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 32

2 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b5cf95178314
fix flexible space widget disappearing when adding to toolbar r=Gijs
Assignee

Comment 33

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

Comment 34

2 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/b5cf95178314
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Is that something that you will request an uplift for?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 37

2 years ago
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
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8913989 - Flags: approval-mozilla-beta?
Comment on attachment 8913989 [details]
Bug 1396953 - fix flexible space widget disappearing when adding to toolbar

Polish photon, taking it.
Attachment #8913989 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Marking verified fixed per comment 40.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.