1665237 - Managed bookmarks UI increases browser.xhtml size unnecessarily

Reporter

Description

•

4 years ago

The managed bookmarks button is always present in the DOM, as is the context menu popup, which offers broadly the same options as the normal places context menu popup but has been duplicated, for reasons that I do not fully understand.

However, for 90% of users neither is ever used.

These nodes should not be in the initially loaded DOM.

:Gijs (he/him)

Reporter

Updated

•

4 years ago

Blocks: 1624683

Mike Kaply [:mkaply]

Assignee

Comment 1

•

4 years ago

Is there some explanation somewhere of how to use html:template?

As far as the context menu goes, I originally tried to reuse it but the functions are completely different and I couldn't come come to with a clever way to do it.

BugBot [:suhaib / :marco/ :calixte]

Comment 2

•

4 years ago

Set release status flags based on info from the regressing bug 1498688

status-firefox80: --- → unaffected

status-firefox81: --- → unaffected

status-firefox-esr68: --- → unaffected

status-firefox-esr78: --- → unaffected

:Gijs (he/him)

Reporter

Comment 3

•

4 years ago

(In reply to Mike Kaply [:mkaply] from comment #1)

Is there some explanation somewhere of how to use html:template?

html:template is just an easy way to avoid changing too much for existing features as the perf team has gone through them. Broadly, it avoids running custom element initialization, and when we actually need the content of the template we can run template.replaceWith(template.content) and that works.

This method has the downside that it's verbose if everything gets its own template, and that it still pays parsing and DOM memory/CPU costs for the contents of the template to be put in the fragment associated with the template node.

In this case, I'd suggest we don't put the content in the DOM at all and only create it dynamically when the policy is in effect. The customizableui code will persist the location of the item. If we want to actually have the item be valid, we could insert the content into gNavToolbox.palette (itself a template), and then call CustomizableUI.ensureWidgetPlacedInWindow and CUI will take care of putting it in the right place.

The only thing I'm not sure about is how this works together with the removable=false bits.

As far as the context menu goes, I originally tried to reuse it but the functions are completely different and I couldn't come come to with a clever way to do it.

Why are the functions completely different? Aren't they both running against bookmarks? Or are the "managed" bookmarks not "real" bookmarks or something?

As for clever, I guess I would have done something like sharing a popupshowing callback, and then using let isManaged = !!event.target.triggerNode.closest("#managed-bookmarks"); to determine if the menu was invoked on the managed bookmarks item or subitems, and hide any items that aren't appropriate for managed bookmarks. It looks like then, because the bookmarks aren't real, to make the items work, the managed bookmarks would want to shim a "fake" places node that has the appropriate data in https://searchfox.org/mozilla-central/rev/30e70f2fe80c97bfbfcd975e68538cefd7f58b2a/browser/components/places/content/browserPlacesViews.js#153-160 . Then everything else should "just work". Really, maybe it'd be better to implement your own fake "places view" for the menu, but that's probably more involved than hacking up the main view...

Anyway, that'd reduce the duplication. Perhaps a simpler option would be to insert the context menu using JS DOM creation APIs iff we're using the managed bookmarks.

Mike Kaply [:mkaply]

Assignee

Comment 4

•

4 years ago

(In reply to :Gijs (he/him) from comment #3)

(In reply to Mike Kaply [:mkaply] from comment #1)

As far as the context menu goes, I originally tried to reuse it but the functions are completely different and I couldn't come come to with a clever way to do it.

Why are the functions completely different? Aren't they both running against bookmarks? Or are the "managed" bookmarks not "real" bookmarks or something?

They are not real bookmarks.

As for clever, I guess I would have done something like sharing a popupshowing callback, and then using let isManaged = !!event.target.triggerNode.closest("#managed-bookmarks"); to determine if the menu was invoked on the managed bookmarks item or subitems, and hide any items that aren't appropriate for managed bookmarks. It looks like then, because the bookmarks aren't real, to make the items work, the managed bookmarks would want to shim a "fake" places node that has the appropriate data in https://searchfox.org/mozilla-central/rev/30e70f2fe80c97bfbfcd975e68538cefd7f58b2a/browser/components/places/content/browserPlacesViews.js#153-160 . Then everything else should "just work". Really, maybe it'd be better to implement your own fake "places view" for the menu, but that's probably more involved than hacking up the main view...

My first attempt at this was using a shimmed places node, but it got really complicated really fast, and there were a number of things that just wouldn't work unless it was a real places node without rewriting significant pieces of code, particularly the code that determined whether or not a places node was a container. All of that code is heavily dependent on being able to query the places database.

And the reason these aren't real bookmarks nodes is because there's no easy way to have bookmarks that don't sync, so this implementation was a compromise.

Anyway, that'd reduce the duplication. Perhaps a simpler option would be to insert the context menu using JS DOM creation APIs if we're using the managed bookmarks.

I'll take a look at that.

Mike Kaply [:mkaply]

Assignee

Comment 5

•

4 years ago

Attached file Bug 1665237 - Make managed bookmarks item dynamic. r?Gijs! — Details

Phabricator Automation

Updated

•

4 years ago

Assignee: nobody → mozilla

Status: NEW → ASSIGNED

Mike Kaply [:mkaply]

Assignee

Comment 6

•

4 years ago

The customizableui code will persist the location of the item. If we want to actually have the item be valid, we could insert the content into gNavToolbox.palette (itself a template), and then call CustomizableUI.ensureWidgetPlacedInWindow and CUI will take care of putting it in the right place.

Severity: -- → S3

Priority: -- → P3

Pulsebot

Comment 7

•

4 years ago

Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/034825cf1c71
Make managed bookmarks item dynamic. r=Gijs

Natalia Csoregi [:nataliaCs]

Comment 8

•

4 years ago

Backed out for multiple failures e.g. browser_1007336_lwthemes_in_customize_mode.js

backout: https://hg.mozilla.org/integration/autoland/rev/7dc798b8981461fd31d54faa61606a938ac29f41

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=034825cf1c71f664ea786c6f4790d029c842ec77&group_state=expanded

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=316399742&repo=autoland&lineNumber=4391

[task 2020-09-22T18:32:25.386Z] 18:32:25 INFO - TEST-START | browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js
[task 2020-09-22T18:32:25.438Z] 18:32:25 INFO - TEST-INFO | started process screencapture
[task 2020-09-22T18:32:25.597Z] 18:32:25 INFO - TEST-INFO | screencapture: exit 0
[task 2020-09-22T18:32:25.597Z] 18:32:25 INFO - Buffered messages logged at 18:32:25
[task 2020-09-22T18:32:25.597Z] 18:32:25 INFO - Entering test bound
[task 2020-09-22T18:32:25.597Z] 18:32:25 INFO - Buffered messages finished
[task 2020-09-22T18:32:25.597Z] 18:32:25 INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js | Reset button should start out disabled -
[task 2020-09-22T18:32:25.597Z] 18:32:25 INFO - Stack trace:
[task 2020-09-22T18:32:25.597Z] 18:32:25 INFO - chrome://mochikit/content/browser-test.js:test_ok:1304
[task 2020-09-22T18:32:25.597Z] 18:32:25 INFO - chrome://mochitests/content/browser/browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js:null:38
[task 2020-09-22T18:32:25.598Z] 18:32:25 INFO - TEST-PASS | browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js | Button should show default theme thumbnail - was: "url("resource://default-theme/icon.svg")" -

Flags: needinfo?(mozilla)

Mike Kaply [:mkaply]

Assignee

Comment 9

•

4 years ago

So this got backed out because of this line in customizableUI.jsm:

https://searchfox.org/mozilla-central/rev/35d927df97900a57ecb562ad13909e392440b0fb/browser/components/customizableui/CustomizableUI.jsm#279

    this.registerArea(
      CustomizableUI.AREA_BOOKMARKS,
      {
        type: CustomizableUI.TYPE_TOOLBAR,
        defaultPlacements: ["managed-bookmarks", "personal-bookmarks"],
        defaultCollapsed: true,
      },
      true
    );

Which caused the Customizable page to always believe something was customized (because managed-bookmarks was in default placements, but wasn't actually there).

Gijs: Any idea how I can do this better? I don't want to insert the button directly into the personal toolbar because the user can move the button. Is there something I'm missing?

Flags: needinfo?(mozilla) → needinfo?(gijskruitbosch+bugs)

Mike Kaply [:mkaply]

Assignee

Comment 10

•

4 years ago

Actually, I think I figured it out. I needed something like:

          // Add button if it doesn't exist
          if (!CustomizableUI.getPlacementOfWidget("managed-bookmarks")) {
            CustomizableUI.addWidgetToArea(
              "managed-bookmarks",
              CustomizableUI.AREA_BOOKMARKS,
              0
            );
          }

and I removed it from defaults.

Gijs, can you confirm?

I also pushed the patch to phabricator.

:Gijs (he/him)

Reporter

Comment 11

•

4 years ago

(In reply to Mike Kaply [:mkaply] from comment #10)

Actually, I think I figured it out. I needed something like:
          // Add button if it doesn't exist
          if (!CustomizableUI.getPlacementOfWidget("managed-bookmarks")) {
            CustomizableUI.addWidgetToArea(
              "managed-bookmarks",
              CustomizableUI.AREA_BOOKMARKS,
              0
            );
          }
and I removed it from defaults.

Gijs, can you confirm?

I also pushed the patch to phabricator.

Ah, hm, this is unfortunate. I think this works, but I'm not sure what's going to happen if a user that has the managed bookmarks enabled resets to default using the button in customize mode? In theory that should remove the node, as it won't be in the default placement list after your changes. But the node is marked as removable=false, so I suspect it won't be able to. I don't know if that's going to upset anything else...

Do all the CUI tests pass locally?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mozilla)

Mike Kaply [:mkaply]

Assignee

Comment 12

•

4 years ago

Ah, hm, this is unfortunate. I think this works, but I'm not sure what's going to happen if a user that has the managed bookmarks enabled resets to default using the button in customize mode? In theory that should remove the node, as it won't be in the default placement list after your changes. But the node is marked as removable=false, so I suspect it won't be able to. I don't know if that's going to upset anything else...

Just tested that and it's works as you said. Because we're marked removable, it actually has no effect at all. So we should be good. The personal toolbar gets hidden and the restore defaults button is disabled.

Do all the CUI tests pass locally?

Yes, which I would expect now because we don't touch anything unless you turn on managed bookmarks.

Flags: needinfo?(mozilla)

Mike Kaply [:mkaply]

Assignee

Comment 13

•

4 years ago

Gijs:

Are you ok with me checking in this fix?

Flags: needinfo?(gijskruitbosch+bugs)

:Gijs (he/him)

Reporter

Comment 14

•

4 years ago

(In reply to Mike Kaply [:mkaply] from comment #13)

Gijs:

Are you ok with me checking in this fix?

Yes, please land this. :-)

Flags: needinfo?(gijskruitbosch+bugs)

Pulsebot

Comment 15

•

4 years ago

Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/2b2e72856dc1
Make managed bookmarks item dynamic. r=Gijs

Razvan Maries

Comment 16

•

4 years ago

•

Edited

Backed out for perma failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=AVCfYOVPT4S_Jl0G8ukzdg.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=baa88a6bec9628a65ea631afe0cd3e0134ea5321&failure_classification_id=2

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=316674288&repo=autoland&lineNumber=15348

TV failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=cBtmUJJHTMmie6TPn1QI4Q.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=2b2e72856dc138c848da111f81a83456360c671c&searchStr=tv

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=316669224&repo=autoland&lineNumber=2291

Backout: https://hg.mozilla.org/integration/autoland/rev/acc569599c8fbfd6c284d87b2a67de9ed968fbcd

Flags: needinfo?(mozilla)

Julien Cristau [:jcristau]

Updated

•

4 years ago

status-firefox82: affected → wontfix

status-firefox83: --- → affected

Pulsebot

Comment 17

•

4 years ago

Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/7e9a6ef4ba02
Make managed bookmarks item dynamic. r=Gijs

Razvan Maries

Comment 18

•

4 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/7e9a6ef4ba02

Status: ASSIGNED → RESOLVED

Closed: 4 years ago

status-firefox83: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → 83 Branch

Mike Kaply [:mkaply]

Assignee

Updated

•

4 years ago

Flags: needinfo?(mozilla)

BMO Automation

Updated

•

4 years ago

Has Regression Range: --- → yes