Closed Bug 1428938 Opened 2 years ago Closed 2 years ago

Remove legacy toolbar customization code

Categories

(Toolkit :: XUL Widgets, task)

task
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/toolbar.xml

There seems to be support for adding custom toolbars which was a feature we removed around Firefox Australis, or maybe before that.

Only Thunderbird and SeaMonkey still support that feature.

We should look into removing that code from Firefox.
Assignee: nobody → ntim.bugs
We should even go further and remove customizeToolbar.xul from the tree, as only comm-central uses it.
Comment on attachment 8941184 [details]
Bug 1428938 - Remove legacy toolbar customization code.

https://reviewboard.mozilla.org/r/211456/#review217314

Try is... unhappy, to say the least, so I don't think this is ready for review.

There's a bunch of stuff in CustomizableUI.jsm that this patch turns into dead code, or that could be simplified. See the work done in bug 975719 to see where that stuff lives. There's also some helpers in head.js that are orphaned by your patch. And we'll need to add a UI migration in CUI to remove persisted toolbar gunk from both XULStore and the CUI JSON thing.

I'm pretty OK with removing support for grandfathered-in toolbars here without telemetry given how late this even became an issue in the Australis lifecycle, and how long it's been since then, and the fact that we'd remove these toolbars on reset and there's no way to get them back except...


... CTR. Which apparently let you add these as recently as 56. Like, I think it is still OK to remove this, but perhaps we'll need to relnote it. It can't really come as a surprise to those users as we've never properly supported the additional toolbars. Please get a second opinion from Jared to make sure, because making a decision like this without data isn't something we should take lightly.
Attachment #8941184 - Flags: review?(gijskruitbosch+bugs) → review-
Summary: Remove unused methods from toolbar bindings → Remove legacy toolbar customization code
Comment on attachment 8941184 [details]
Bug 1428938 - Remove legacy toolbar customization code.

https://reviewboard.mozilla.org/r/211456/#review217456

Clearing review for now per discussion on IRC.
Attachment #8941184 - Flags: review?(gijskruitbosch+bugs)
Attachment #8941184 - Flags: review?(gijskruitbosch+bugs)
The patch is now ready for review :)
Apart from the l10n thing (which according to :flod seems fine), try seems green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7157251478ca
Comment on attachment 8941184 [details]
Bug 1428938 - Remove legacy toolbar customization code.

https://reviewboard.mozilla.org/r/211456/#review217510

Almost there, but I think there's a few loose ends, see below.

::: browser/base/content/browser.js:5324
(Diff revision 12)
>  };
>  
>  function getTogglableToolbars() {
>    let toolbarNodes = Array.slice(gNavToolbox.childNodes);
> -  toolbarNodes = toolbarNodes.concat(gNavToolbox.externalToolbars);
>    toolbarNodes = toolbarNodes.filter(node => node.getAttribute("toolbarname"));

I don't think this filter will be required anymore, unless there are builtin toolbars that don't have a "toolbarname" attribute?

::: browser/components/places/content/places.xul:145
(Diff revision 12)
>      <menupopup id="placesColumnsContext"
>                 onpopupshowing="ViewMenu.fillWithColumns(event, null, null, 'checkbox', null);"
>                 oncommand="ViewMenu.showHideColumn(event.target); event.stopPropagation();"/>
>    </popupset>
>  
> -  <toolbox id="placesToolbox">
> +  <vbox id="placesToolbox">

Why the node name change here? You haven't done this in browser.xul... I guess as long as it has no visual impact I don't mind, but it's a little surprising (and I can't quickly verify the visual impact right now).

::: toolkit/content/widgets/toolbar.xml:20
(Diff revision 12)
>        <field name="palette">
>          null
>        </field>

Want to file a followup bug to move this to an expando property that contains a document fragment instead? Or perhaps just remove the concept of a toolbox node entirely? CustomizableUI should be able to deal with this for Firefox.

::: toolkit/content/widgets/toolbar.xml:29
(Diff revision 12)
>        <property name="toolbarName"
>                  onget="return this.getAttribute('toolbarname');"
>                  onset="this.setAttribute('toolbarname', val); return val;"/>

This property is dead, as far as I can tell from DXR. Which actually means, again as far as I can tell, that we can remove this binding and flatten it into toolbar-base .
Attachment #8941184 - Flags: review?(gijskruitbosch+bugs)
Blocks: 1389034
Duplicate of this bug: 1389034
Comment on attachment 8941184 [details]
Bug 1428938 - Remove legacy toolbar customization code.

https://reviewboard.mozilla.org/r/211456/#review217536

::: toolkit/content/widgets/toolbar.xml:29
(Diff revision 12)
>        <property name="toolbarName"
>                  onget="return this.getAttribute('toolbarname');"
>                  onset="this.setAttribute('toolbarname', val); return val;"/>

I think I'd handle this in other bugs, because:

1) We'll also need to copy the role into XULMap.h before removing this binding. This can be done in Bug 1428930.
2) Once we get UA sheets sorted out in Bug 1420229 we will be able to load toolbar.css there, removing the need for toolbar-base.

Once those two are done we could completely remove toolbar and toolbar-base.  We could handle (1) here - I wouldn't argue against it - but it'd be a bit of an odd state to end up binding toolbar-base onto the  toolbar element in CSS.
I mean, either way we'd still have the 'special' CUI toolbar bindings in browser/, which have more behaviour that'd need to be factored out. But I'm fine leaving the actual removal of the bindings to a separate bug if that's more convenient.
(In reply to :Gijs from comment #21)
> Comment on attachment 8941184 [details]
> Bug 1428938 - Remove legacy toolbar customization code.
> 
> https://reviewboard.mozilla.org/r/211456/#review217510
> 
> Almost there, but I think there's a few loose ends, see below.
> 
> ::: browser/base/content/browser.js:5324
> (Diff revision 12)
> >  };
> >  
> >  function getTogglableToolbars() {
> >    let toolbarNodes = Array.slice(gNavToolbox.childNodes);
> > -  toolbarNodes = toolbarNodes.concat(gNavToolbox.externalToolbars);
> >    toolbarNodes = toolbarNodes.filter(node => node.getAttribute("toolbarname"));
> 
> I don't think this filter will be required anymore, unless there are builtin
> toolbars that don't have a "toolbarname" attribute?

Done.

> ::: browser/components/places/content/places.xul:145
> (Diff revision 12)
> >      <menupopup id="placesColumnsContext"
> >                 onpopupshowing="ViewMenu.fillWithColumns(event, null, null, 'checkbox', null);"
> >                 oncommand="ViewMenu.showHideColumn(event.target); event.stopPropagation();"/>
> >    </popupset>
> >  
> > -  <toolbox id="placesToolbox">
> > +  <vbox id="placesToolbox">
> 
> Why the node name change here? You haven't done this in browser.xul... I
> guess as long as it has no visual impact I don't mind, but it's a little
> surprising (and I can't quickly verify the visual impact right now).

I didn't see any visual change on macOS, but I haven't verified elsewhere. Reverted this change for now.

> ::: toolkit/content/widgets/toolbar.xml:20
> (Diff revision 12)
> >        <field name="palette">
> >          null
> >        </field>
> 
> Want to file a followup bug to move this to an expando property that
> contains a document fragment instead? Or perhaps just remove the concept of
> a toolbox node entirely? CustomizableUI should be able to deal with this for
> Firefox.

Bug 1429464.

> ::: toolkit/content/widgets/toolbar.xml:29
> (Diff revision 12)
> >        <property name="toolbarName"
> >                  onget="return this.getAttribute('toolbarname');"
> >                  onset="this.setAttribute('toolbarname', val); return val;"/>
> 
> This property is dead, as far as I can tell from DXR. Which actually means,
> again as far as I can tell, that we can remove this binding and flatten it
> into toolbar-base .

Removed the toolbarName property but kept the toolbar binding since it's needed for a11y. The a11y part can be handled in bug 1428930.
Comment on attachment 8941184 [details]
Bug 1428938 - Remove legacy toolbar customization code.

https://reviewboard.mozilla.org/r/211456/#review217622

::: browser/base/content/browser.js:5323
(Diff revisions 12 - 14)
>      return CanCloseWindow();
>    },
>  };
>  
>  function getTogglableToolbars() {
> -  let toolbarNodes = Array.slice(gNavToolbox.childNodes);
> +  return [...gNavToolbox.childNodes];

Heh, I keep finding more simplifications... hopefully this will be the last one. But you can basically:

1) remove this function
2) make the callsite in browser.js itself do:

```js
let toolbarNodes = gNavToolbox.childNodes;
```

directly.
3) make the callsite in CUI omit the call, the if statement that uses the length of togglable toolbars (currently always >= 1 anyway) and even the constant defined at the top of the file for the toolbar show/hide menu button's ID.
Attachment #8941184 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #7)
> I'm pretty OK with removing support for grandfathered-in toolbars here
> without telemetry given how late this even became an issue in the Australis
> lifecycle, and how long it's been since then, and the fact that we'd remove
> these toolbars on reset and there's no way to get them back except...
> 
> 
> ... CTR. Which apparently let you add these as recently as 56. Like, I think
> it is still OK to remove this, but perhaps we'll need to relnote it. It
> can't really come as a surprise to those users as we've never properly
> supported the additional toolbars. Please get a second opinion from Jared to
> make sure, because making a decision like this without data isn't something
> we should take lightly.

--> ni :jaws for this.
Flags: needinfo?(jaws)
+1 for removing this from me.

This will not fully stop the userchrome.js hack (which was never official nor publicly supported) from working, though I am sure that people will find a way around it and keep it working. userchrome.js will eventually stop working altogether when we remove XBL down the road.
Flags: needinfo?(jaws)
Comment on attachment 8941184 [details]
Bug 1428938 - Remove legacy toolbar customization code.

https://reviewboard.mozilla.org/r/211456/#review217816

::: browser/components/customizableui/CustomizeMode.jsm:281
(Diff revision 15)
> -      let toolbarVisibilityBtn = document.getElementById(kToolbarVisibilityBtn);
> -      let togglableToolbars = window.getTogglableToolbars();
> -      if (togglableToolbars.length == 0) {
> +      let toolbarVisibilityBtn =
> +        document.getElementById("customization-toolbar-visibility-button");
> +      if (window.gNavToolbox.childNodes.length == 0) {
>          toolbarVisibilityBtn.setAttribute("hidden", "true");
>        } else {
>          toolbarVisibilityBtn.removeAttribute("hidden");
>        }

Please just remove this entire block.

The button doesn't have a hidden attribute by default, and none is re-added when leaving customize mode, and the length of childnodes for the navtoolbox is never 0.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 228e8e730d82 -d ba4ce1d35c2d: rebasing 441959:228e8e730d82 "Bug 1428938 - Remove legacy toolbar customization code. r=Gijs" (tip)
merging browser/base/content/browser.js
merging browser/components/nsBrowserGlue.js
merging toolkit/content/jar.mn
merging toolkit/content/tests/chrome/chrome.ini
merging toolkit/locales/jar.mn
merging toolkit/themes/osx/global/jar.mn
merging toolkit/themes/shared/non-mac.jar.inc.mn
warning: conflicts while merging browser/components/nsBrowserGlue.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/themes/shared/non-mac.jar.inc.mn! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e08ec3e15b9b
Remove legacy toolbar customization code. r=Gijs
Depends on: 1429861
Depends on: 1429862
https://hg.mozilla.org/mozilla-central/rev/e08ec3e15b9b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
TypeError: toolbox.externalToolbars is undefined[Learn More]  toolbar.xml:145:1
Depends on: 1430009
(In reply to nayinain from comment #38)
> TypeError: toolbox.externalToolbars is undefined[Learn More] 
> toolbar.xml:145:1

Thanks for catching that! Filed bug 1430009
Blocks: 1430128
Blocks: 679724
Duplicate of this bug: 1343833
Duplicate of this bug: 1363366
Depends on: 1451708
Blocks: 1451708
No longer depends on: 1451708
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.