Closed Bug 1453281 Opened 2 years ago Closed 2 years ago

Remove obsolete menubar binding and styling

Categories

(Toolkit :: XUL Widgets, task)

task
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file)

No description provided.
Can you provide some more info on this? What was <menubar> replaced with? I see a few occurrences even in m-c.
I'm not going to remove the element but the binding which doesn't do anything useful.
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.
Attachment #8966951 - Flags: review?(paolo.mozmail) → review+
Attachment #8966951 - Flags: review?(bgrinstead)
(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 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?
(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.
(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 on attachment 8966951 [details]
Bug 1453281 - Remove obsolete menubar binding and styling.

https://reviewboard.mozilla.org/r/235632/#review241520
Attachment #8966951 - Flags: review?(bgrinstead) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a26b1da04e5
Remove obsolete menubar binding and styling. r=bgrins,Paolo
https://hg.mozilla.org/mozilla-central/rev/7a26b1da04e5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1455496
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.