1453281 - Remove obsolete menubar binding and styling

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task)

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Can you provide some more info on this? What was <menubar> replaced with? I see a few occurrences even in m-c.

Comment 2

[Dão Gottwald [:dao]]

I'm not going to remove the element but the binding which doesn't do anything useful.

Comment 3

[Dão Gottwald [:dao]]

Attachment: Bug 1453281 - Remove obsolete menubar binding and styling.

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

Comment 4

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a19849274bf5ddc21ecceeaddb9f4657f4fde8f

Comment 5

[:Paolo Amadini]

Comment on attachment 8966951 [details]
Bug 1453281 - Remove obsolete menubar binding and styling.

https://reviewboard.mozilla.org/r/235632/#review241438

Looks good, but someone else should probably review the devtools bits. Asking Brian but feel free to redirect.

::: browser/themes/windows/places/organizer.css
(Diff revision 1)
>    transform: scaleX(-1);
>  }
>  
>  /* Menu */
> -#placesMenu {
> -  margin-inline-start: 6px;

nit: we may want to keep this margin after the navigation buttons in the Library.

::: toolkit/themes/windows/global/toolbar.css
(Diff revision 1)
>  }
>  
>  /* ::::: lightweight theme ::::: */
>  
> -menubar:-moz-lwtheme,
> -toolbox:-moz-lwtheme,

"toolbox" still has "-moz-appearance: toolbox;", so I'm not sure this can be removed. What does it do, is it just selecting a system background color?

If this is not needed anymore, maybe the support for this value of the "appearance" property could be removed together with the selectors, here or in a separate bug.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8966951 [details]
Bug 1453281 - Remove obsolete menubar binding and styling.

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

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #5)
> ::: browser/themes/windows/places/organizer.css
> (Diff revision 1)
> >    transform: scaleX(-1);
> >  }
> >  
> >  /* Menu */
> > -#placesMenu {
> > -  margin-inline-start: 6px;
> 
> nit: we may want to keep this margin after the navigation buttons in the
> Library.

Oops. Removed this by accident.

> ::: toolkit/themes/windows/global/toolbar.css
> (Diff revision 1)
> >  }
> >  
> >  /* ::::: lightweight theme ::::: */
> >  
> > -menubar:-moz-lwtheme,
> > -toolbox:-moz-lwtheme,
> 
> "toolbox" still has "-moz-appearance: toolbox;", so I'm not sure this can be
> removed. What does it do, is it just selecting a system background color?

Good catch. https://hg.mozilla.org/mozilla-central/rev/df6d2c3ee671e81f1a8b4f6d13197eee52db9680 moved -moz-appearance: toolbox to global.css, but toolbar.css is now loaded via components.css so I'll move it back there.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8966951 [details]
Bug 1453281 - Remove obsolete menubar binding and styling.

https://reviewboard.mozilla.org/r/235632/#review241506

::: devtools/client/scratchpad/scratchpad.xul:144
(Diff revision 2)
>         command="key_gotoLine"
>         modifiers="accel"/>
>  
>  </keyset>
>  
> +<toolbar type="menubar">

What's the guideline for when to wrap a menubar with a `<toolbar type="menubar">?

In particular, why do we need to do this for scratchpad/webide/layoutdebugger, but not in places.xul https://searchfox.org/mozilla-central/source/browser/components/places/content/places.xul#189 or browser-menubar.inc?

Comment 9

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> ::: devtools/client/scratchpad/scratchpad.xul:144
> (Diff revision 2)
> >         command="key_gotoLine"
> >         modifiers="accel"/>
> >  
> >  </keyset>
> >  
> > +<toolbar type="menubar">
> 
> What's the guideline for when to wrap a menubar with a `<toolbar
> type="menubar">?

Basically always, as of this bug.

> In particular, why do we need to do this for
> scratchpad/webide/layoutdebugger, but not in places.xul
> https://searchfox.org/mozilla-central/source/browser/components/places/
> content/places.xul#189 or browser-menubar.inc?

Both are already in a toolbar, except for macWindow.inc.xul where it doesn't matter because we don't render it. places.xul doesn't use type="menubar" because it doesn't want the associated style.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [::dao] from comment #9)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > ::: devtools/client/scratchpad/scratchpad.xul:144
> > (Diff revision 2)
> > >         command="key_gotoLine"
> > >         modifiers="accel"/>
> > >  
> > >  </keyset>
> > >  
> > > +<toolbar type="menubar">
> > 
> > What's the guideline for when to wrap a menubar with a `<toolbar
> > type="menubar">?
> 
> Basically always, as of this bug.

OK. I do wonder if we should get rid of menubar entirely (i.e. support having <menu> and related tags directly beneath <toolbar type="menubar"> and also have platform integration for detecting menubars on osx look for <toolbar type="menubar">). But I don't think it needs to block here - especially since we are still working to figure out how to support menus in HTML docs which may end up changing things.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8966951 [details]
Bug 1453281 - Remove obsolete menubar binding and styling.

https://reviewboard.mozilla.org/r/235632/#review241520

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a26b1da04e5
Remove obsolete menubar binding and styling. r=bgrins,Paolo

Comment 13

[Eliza Balazs [:ebalazs_]]

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