Closed Bug 1354117 Opened 7 years ago Closed 7 years ago

On startup of a photon browser, migrate customized items from the hamburger panel to the overflow panel

Categories

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

53 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.1 - Aug 15
Tracking Status
firefox57 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-structure])

Attachments

(15 files)

59 bytes, text/x-review-board-request
johannh
: review+
Details
59 bytes, text/x-review-board-request
johannh
: review+
Details
59 bytes, text/x-review-board-request
johannh
: review+
Details
59 bytes, text/x-review-board-request
adw
: review+
Details
59 bytes, text/x-review-board-request
adw
: review+
Details
59 bytes, text/x-review-board-request
adw
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
johannh
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
gasolin
: review+
Details
59 bytes, text/x-review-board-request
kmag
: review+
Details
59 bytes, text/x-review-board-request
adw
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
See bug 1354083.

When browser.photon.menusRefresh (or whatever) is true, and wasn't the previous run, CustomizableUI should:

- get a list of items that were in the hamburger menu
- remove all the items that are in the default set for the hamburger menu
- append those items to the overflow menu set
- remove the stored data for the hamburger menu
- store the data again

When the pref is false, and wasn't the previous run, we do the opposite:
- get a list of items that were in the permanent overflow menu
- remove all the items that are in the default set for the *hamburger* menu
- append those items to the hamburger menu set
- remove the stored data for the overflow menu
- store the data again

This will lose ordering information and removed items out of default-set hamburger menu items if you go from australis->photon->australis. That seems an acceptable trade-off given that we'll need to store a lot more stuff if we want to keep that (and ordering/removal seems significantly less important than customized/added items generally).

This will not only help while we're developing and channel-migrating, but should also do the Right Thing for add-on buttons from existing profiles on the first Photon startup (though once we're confident we're shipping we should probably remove the pref...). Seeing as we need to do that kind of thing once already for a UI migration, putting this into CUI.jsm and doing testing with it seems like the sensible thing to do.

Potential bonus: in theory we could do this at runtime with a pref observer, too, if we ever end up supporting flipping the photon prefs at runtime (not a priority, though).
QA Whiteboard: [photon]
QA Whiteboard: [photon]
Whiteboard: [photon]
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Priority: P2 → P3
Whiteboard: [photon-structure] → [reserve-photon-structure]
That should actually have been blocking the new menu being enabled by default in Nightly to avoid bad UX (by having to add the customized items manually again).

For that reason it is at least a must-have when the new menu reaches Stable (or even Beta).

Sebastian
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]
Blocks: 1378807
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.4 - Aug 1
Priority: P2 → P1
So this doesn't quite remove everything pre-photon. Specifically, things that should be removed in a followup:

- vestigial uses of CustomizableUI.AREA_PANEL. I thought I'd scope-creeped this bug enough.
- the XUL for the old panel
- a lot of CSS (notably anything mentioning PanelUI-contents, panelUI-grid, etc.)
Comment on attachment 8892610 [details]
Bug 1354117 - don't force border radius to 0 for photon (x-ref bug 1374315), fix webextension tests,

https://reviewboard.mozilla.org/r/163570/#review168998

redirect to kris
Attachment #8892610 - Flags: review?(aswan) → review?(kmaglione+bmo)
Comment on attachment 8892609 [details]
Bug 1354117 - remove photon checks from UITour,

https://reviewboard.mozilla.org/r/163568/#review169106

for query and widgetName attribute, we can simplify the call to just return necessary ids

::: browser/components/uitour/UITour.jsm:114
(Diff revision 1)
>                                                          "class",
>                                                          "toolbarbutton-icon");
>        },
> -      // This is a fake widgetName starting with the "PanelUI-"/"appMenu-" prefix so we know
> +      // This is a fake widgetName starting with the "appMenu-" prefix so we know
>        // to automatically open the appMenu when annotating this target.
>        get widgetName() {

can simplify to `widgetName: "appMenu-fxa-label",`

::: browser/components/uitour/UITour.jsm:119
(Diff revision 1)
>        get widgetName() {
> -        return gPhotonStructure ? "appMenu-fxa-label" : "PanelUI-fxa-label";
> +        return "appMenu-fxa-label";
>        },
>      }],
>      ["addons", {
>        query(aDocument) {

can simplify to query: "#appMenu-addons-button",

::: browser/components/uitour/UITour.jsm:147
(Diff revision 1)
> -      get widgetName() {
> -        return gPhotonStructure ? "appMenu-customize-button" : "PanelUI-customize";
> -      },
>      }],
>      ["devtools", {
>        query(aDocument) {

query: "#appMenu-developer-button",

::: browser/components/uitour/UITour.jsm:154
(Diff revision 1)
> -        if (button || !gPhotonStructure) {
> +        if (button) {
>            return button;
>          }
>          return aDocument.getElementById("appMenu-developer-button");
>        },
>        get widgetName() {

widgetName: "appMenu-developer-button",

::: browser/components/uitour/UITour.jsm:164
(Diff revision 1)
>        allowAdd: true,
>        query: "#panic-button",
>        widgetName: "panic-button",
>      }],
>      ["help", {
>        query: (aDocument) => {

query: "#appMenu-help-button",
Attachment #8892609 - Flags: review?(gasolin)
Attachment #8892609 - Flags: review?(gasolin)
Attachment #8892610 - Flags: review?(aswan) → review?(kmaglione+bmo)
Comment on attachment 8892609 [details]
Bug 1354117 - remove photon checks from UITour,

For some reason, correcting the r? for kmag cleared the review request here. I updated this cset per the earlier comments, so r?gasolin again.
Attachment #8892609 - Flags: review?(gasolin)
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Comment on attachment 8892601 [details]
Bug 1354117 - update panel button tests for photon,

https://reviewboard.mozilla.org/r/163404/#review169312
Attachment #8892601 - Flags: review?(adw) → review+
Comment on attachment 8892602 [details]
Bug 1354117 - fix or remove tests that drag items to the panel,

https://reviewboard.mozilla.org/r/163406/#review169314
Attachment #8892602 - Flags: review?(adw) → review+
Comment on attachment 8892603 [details]
Bug 1354117 - Update various other customizableui tests for photon,

https://reviewboard.mozilla.org/r/163408/#review169316
Attachment #8892603 - Flags: review?(adw) → review+
Comment on attachment 8892611 [details]
Bug 1354117 - stop relying on gPhotonStructure in bookmarks/history code,

https://reviewboard.mozilla.org/r/163574/#review169318
Attachment #8892611 - Flags: review?(adw) → review+
Comment on attachment 8892610 [details]
Bug 1354117 - don't force border radius to 0 for photon (x-ref bug 1374315), fix webextension tests,

https://reviewboard.mozilla.org/r/163570/#review169530
Attachment #8892610 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8892609 [details]
Bug 1354117 - remove photon checks from UITour,

https://reviewboard.mozilla.org/r/163568/#review169558
Attachment #8892609 - Flags: review?(gasolin) → review+
Comment on attachment 8892600 [details]
Bug 1354117 - update customize mode button state test,

https://reviewboard.mozilla.org/r/163402/#review169692

It's a funny test now, but I think I'm good with either keeping or removing.
Attachment #8892600 - Flags: review?(jhofmann) → review+
Comment on attachment 8892605 [details]
Bug 1354117 - fix telemetry and a number of tests that flip photon prefs,

https://reviewboard.mozilla.org/r/163412/#review169902
Attachment #8892605 - Flags: review?(jhofmann) → review+
Comment on attachment 8892604 [details]
Bug 1354117 - only dispatch view events once, and fix synced tabs button test,

https://reviewboard.mozilla.org/r/163410/#review169984

::: browser/components/customizableui/PanelMultiView.jsm:678
(Diff revision 2)
> -    if (this.panelViews) {
> -      let custWidget = CustomizableWidgets.find(widget => widget.viewId == viewNode.id);
> +    if (eventName.startsWith("View")) {
> +      // Don't need to do this for PanelMultiViewHidden

Can we change this to
if (eventName != "PanelMultiViewHidden") {
  ...
}
Attachment #8892604 - Flags: review?(jaws) → review+
Comment on attachment 8892606 [details]
Bug 1354117 - remove old panel code from CustomizableUI, add migration,

https://reviewboard.mozilla.org/r/163414/#review169986

::: browser/components/customizableui/CustomizableUI.jsm:2835
(Diff revision 2)
>     * Constant reference to the ID of the menu panel.
> +   * DEPRECATED.
>     */
>    AREA_PANEL: "PanelUI-contents",

What's the point of keeping this around? (Or maybe it's removed in a later patch?)
Attachment #8892606 - Flags: review?(jaws) → review+
Comment on attachment 8892607 [details]
Bug 1354117 - remove old panel code from customize mode and update related CSS,

https://reviewboard.mozilla.org/r/163416/#review169990

::: browser/components/customizableui/CustomizeMode.jsm
(Diff revision 2)
> -        // Hide the palette before starting the transition for increased perf.
> -        this.visiblePalette.hidden = true;
> -        this.visiblePalette.removeAttribute("showing");

Shouldn't we still do this?
Attachment #8892607 - Flags: review?(jaws) → review+
Comment on attachment 8892608 [details]
Bug 1354117 - remove old panel code from panelUI and CSS,

https://reviewboard.mozilla.org/r/163418/#review169996

::: browser/themes/shared/customizableui/panelUI.inc.css:32
(Diff revision 2)
>  #PanelUI-popup #PanelUI-contents:empty {
>    height: 128px;
>  }
>  
>  #PanelUI-popup #PanelUI-contents:empty::before {

I guess these blocks should be removed too <RIP/>

Was there a reason you kept them?
Attachment #8892608 - Flags: review?(jaws) → review+
Comment on attachment 8892612 [details]
Bug 1354117 - remove pref from firefox.js,

https://reviewboard.mozilla.org/r/163576/#review169998
Attachment #8892612 - Flags: review?(jaws) → review+
Comment on attachment 8892598 [details]
Bug 1354117 - remove panel-specific styling and associated code, include closemenu in styling in overflow panel,

https://reviewboard.mozilla.org/r/163398/#review170192
Attachment #8892598 - Flags: review?(jhofmann) → review+
Comment on attachment 8892599 [details]
Bug 1354117 - remove wrap handling for the bookmarks toolbar button and tests verifying it,

https://reviewboard.mozilla.org/r/163400/#review170216

Yeah, let's remove that test.

As a side note, the bookmark toolbar button looks strange (too short) inside the overflow panel. Your patch isn't responsible for that though, so I guess I'll make a new bug (if there isn't one already).
Attachment #8892599 - Flags: review?(jhofmann) → review+
(In reply to Johann Hofmann [:johannh] from comment #50)
> Comment on attachment 8892599 [details]
> Bug 1354117 - remove wrap handling for bookmarks toolbar button,
> 
> https://reviewboard.mozilla.org/r/163400/#review170216
> 
> Yeah, let's remove that test.
> 
> As a side note, the bookmark toolbar button looks strange (too short) inside
> the overflow panel. Your patch isn't responsible for that though, so I guess
> I'll make a new bug (if there isn't one already).

I had noticed this and filed bug 1386005.
Comment on attachment 8892607 [details]
Bug 1354117 - remove old panel code from customize mode and update related CSS,

https://reviewboard.mozilla.org/r/163416/#review169990

> Shouldn't we still do this?

We hide the palette and remove the showing attribute in exit(), so it shouldn't need to happen here. I just checked, and noticed that this particular container (ie customization-palette) doesn't start with hidden=true in the markup, so I'll add that instead and check that works before landing. :-)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #45)
> Comment on attachment 8892606 [details]
> Bug 1354117 - remove old panel code from CustomizableUI, add migration,
> 
> https://reviewboard.mozilla.org/r/163414/#review169986
> 
> ::: browser/components/customizableui/CustomizableUI.jsm:2835
> (Diff revision 2)
> >     * Constant reference to the ID of the menu panel.
> > +   * DEPRECATED.
> >     */
> >    AREA_PANEL: "PanelUI-contents",
> 
> What's the point of keeping this around? (Or maybe it's removed in a later
> patch?)

Contrary to what the 15 patches might suggest, I haven't quite succeeded in ripping out *everything*:

 (In reply to :Gijs from comment #17)
> So this doesn't quite remove everything pre-photon. Specifically, things
> that should be removed in a followup:
> 
> - vestigial uses of CustomizableUI.AREA_PANEL. I thought I'd scope-creeped
> this bug enough.
> - the XUL for the old panel
> - a lot of CSS (notably anything mentioning PanelUI-contents, panelUI-grid,
> etc.)

but this is enough that automated tests pass and we no longer rely on the photon structure pref. I'll file followup bugs for the rest once this lands + sticks.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e6ba2424ca8c
remove panel-specific styling and associated code, include closemenu in styling in overflow panel, r=johannh
https://hg.mozilla.org/integration/autoland/rev/11a53e10db19
remove wrap handling for the bookmarks toolbar button and tests verifying it, r=johannh
https://hg.mozilla.org/integration/autoland/rev/f6e7d68931d3
update customize mode button state test, r=johannh
https://hg.mozilla.org/integration/autoland/rev/a8502931b53d
update panel button tests for photon, r=adw
https://hg.mozilla.org/integration/autoland/rev/a8891e2183d0
fix or remove tests that drag items to the panel, r=adw
https://hg.mozilla.org/integration/autoland/rev/06eeb43124fa
Update various other customizableui tests for photon, r=adw
https://hg.mozilla.org/integration/autoland/rev/ecf4c3d84404
only dispatch view events once, and fix synced tabs button test, r=jaws
https://hg.mozilla.org/integration/autoland/rev/6e1861380a55
fix telemetry and a number of tests that flip photon prefs, r=johannh
https://hg.mozilla.org/integration/autoland/rev/5fbd1915d5ad
remove old panel code from CustomizableUI, add migration, r=jaws
https://hg.mozilla.org/integration/autoland/rev/274fd6bc863b
remove old panel code from customize mode and update related CSS, r=jaws
https://hg.mozilla.org/integration/autoland/rev/37ad67687701
remove old panel code from panelUI and CSS, r=jaws
https://hg.mozilla.org/integration/autoland/rev/d0e469465f9e
remove photon checks from UITour, r=gasolin
https://hg.mozilla.org/integration/autoland/rev/862e0401146f
don't force border radius to 0 for photon (x-ref bug 1374315), fix webextension tests, r=kmag
https://hg.mozilla.org/integration/autoland/rev/1de22eca4069
stop relying on gPhotonStructure in bookmarks/history code, r=adw
https://hg.mozilla.org/integration/autoland/rev/4fb560842d16
remove pref from firefox.js, r=jaws
Blocks: 1387512
Depends on: 1387851
No longer depends on: 1387851
Depends on: 1388029
https://hg.mozilla.org/projects/date/rev/e6ba2424ca8c799134df7855a9f14309a359ea6c
Bug 1354117 - remove panel-specific styling and associated code, include closemenu in styling in overflow panel, r=johannh

https://hg.mozilla.org/projects/date/rev/11a53e10db19204e6f4a33427ef8b02d0e7691ef
Bug 1354117 - remove wrap handling for the bookmarks toolbar button and tests verifying it, r=johannh

https://hg.mozilla.org/projects/date/rev/f6e7d68931d3e1cb00dd2b8e8e8ccf5ecc37b1e8
Bug 1354117 - update customize mode button state test, r=johannh

https://hg.mozilla.org/projects/date/rev/a8502931b53d010bc63224e4097fa4ba6b3251c6
Bug 1354117 - update panel button tests for photon, r=adw

https://hg.mozilla.org/projects/date/rev/a8891e2183d071d2fb7bb7d7170a0b35420cfd6e
Bug 1354117 - fix or remove tests that drag items to the panel, r=adw

https://hg.mozilla.org/projects/date/rev/06eeb43124facd718fb95cf2a92b658902f32c4b
Bug 1354117 - Update various other customizableui tests for photon, r=adw

https://hg.mozilla.org/projects/date/rev/ecf4c3d8440440aa853aca499524ccd8e70ec83d
Bug 1354117 - only dispatch view events once, and fix synced tabs button test, r=jaws

https://hg.mozilla.org/projects/date/rev/6e1861380a559c3fbdb2f0aae3ff7bd9459eae98
Bug 1354117 - fix telemetry and a number of tests that flip photon prefs, r=johannh

https://hg.mozilla.org/projects/date/rev/5fbd1915d5ad6141df5fa1a299410522c1638918
Bug 1354117 - remove old panel code from CustomizableUI, add migration, r=jaws

https://hg.mozilla.org/projects/date/rev/274fd6bc863b525564f9a51b7716cf7728212ec8
Bug 1354117 - remove old panel code from customize mode and update related CSS, r=jaws

https://hg.mozilla.org/projects/date/rev/37ad67687701f79c72d1dd26122955b50d26acb6
Bug 1354117 - remove old panel code from panelUI and CSS, r=jaws

https://hg.mozilla.org/projects/date/rev/d0e469465f9e9dd6c97ef841830821704ba5135b
Bug 1354117 - remove photon checks from UITour, r=gasolin

https://hg.mozilla.org/projects/date/rev/862e0401146fae86570b1ca59c5bb247b463616b
Bug 1354117 - don't force border radius to 0 for photon (x-ref bug 1374315), fix webextension tests, r=kmag

https://hg.mozilla.org/projects/date/rev/1de22eca4069482cde062792af2b5269cd6ea2b2
Bug 1354117 - stop relying on gPhotonStructure in bookmarks/history code, r=adw

https://hg.mozilla.org/projects/date/rev/4fb560842d169651dba0ddfea66519ff8490a740
Bug 1354117 - remove pref from firefox.js, r=jaws
Depends on: 1390006
I no longer see these photon prefs in 57. Is this something I need to go back and verify in 55/56?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Grover Wimberly IV [:Grover-QA] from comment #72)
> I no longer see these photon prefs in 57. Is this something I need to go
> back and verify in 55/56?

I updated the summary. It would be useful to verify that if you add custom items to the hamburger panel (ie that aren't in it by default) in 55/56, and then open that profile in 57, any additional items you added to the hamburger panel appear (pinned) in the overflow panel. Note that this didn't actually work correctly until a few days ago when bug 1391616 landed. :-(
Depends on: 1391616
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gwimberly)
Summary: On startup check the photon UI pref and migrate customized items from the hamburger panel to the overflow panel or vice versa → On startup of a photon browser, migrate customized items from the hamburger panel to the overflow panel
I have verified this bug fix with the latest nightly on Windows 10. Customized items added in Fx 56 hamburger panel appears in the overflow panel of Fx 57.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(gwimberly)
Depends on: 1455965
You need to log in before you can comment on or make changes to this bug.