Closed Bug 1288406 Opened 8 years ago Closed 8 years ago

Port Firefox's menu bar to FTL

Categories

(L20n :: General, defect)

Product:
L20n
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

(
URL
)

Details

(Whiteboard: [gecko-l20n])

User Story

The first feature that we'll want to port to L20n/FTL in Firefox is the UI that is visible right when a new window opens:

 - the URL bar placeholder ("Search or enter address"),
 - the Search bar placeholder ("Search"),
 - the Menu bar labels ("File", "Edit", "View", etc.), including the menu items.

Attachments

(8 files, 2 obsolete files)

Bug 1288406 - Part 1: Add l20n-chrome-xul.js.
(deleted), text/x-review-board-request
zbraniecki
: review+
Details
Bug 1288406 - Part 2: Add {menubar,toolbar}.ftl files.
(deleted), text/x-review-board-request
Pike
: review+
Details
Bug 1288406 - Part 3: Port browser's menubar to L20n.
(deleted), text/x-review-board-request
zbraniecki
: review+
Pike
: review+
Details
Bug 1288406 - Part 4: Explicitly observe broadcasters' localizable attributes.
(deleted), text/x-review-board-request
Pike
: review+
Details
Bug 1288406 - Part 5: Port branding to FTL.
(deleted), text/x-review-board-request
Pike
: review+
Details
File menu without Quit Firefox
(deleted), image/png
Details
Firefox not active, menu partially not showing
(deleted), image/png
Details
Bug 1288406 - Part 6: Don't use OS() for platform-specific key shortcuts for now.
(deleted), text/x-review-board-request
Pike
: review+
Details 
As per [1] and then [2], the first feature that we'll want to port to L20n/FTL in Firefox is the UI that is visible right when a new window opens:

 - the URL bar placeholder ("Search or enter address"),
 - the Search bar placeholder ("Search"),
 - the Menu bar labels ("File", "Edit", "View", etc.), including the menu items.

[1] https://groups.google.com/d/msg/mozilla.tools.l10n/BjZgzc4qNo4/J6XNBo3YAQAJ
[2] https://wiki.mozilla.org/L10n:Planning/2016-07-19#Tech_Meeting_Notes
Assignee: nobody → stas
Attached patch Patch 1. browser-menu.inc ported to FTL (obsolete) (deleted) — — Splinter Review 
Here's my first take.  It works well and I'm not seeing any FOUCs.  The code is also available at https://github.com/stasm/gecko-dev/tree/l20n-1288406.
Attachment #8773822 - Flags: feedback?(l10n)
Attachment #8773822 - Flags: feedback?(gandalf)
Comment on attachment 8773822 [details] [diff] [review]
Patch 1.  browser-menu.inc ported to FTL

Review of attachment 8773822 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-menubar.inc
@@ +443,4 @@
>                  <menupopup id="menuWebDeveloperPopup">
>                    <menuitem id="menu_pageSource"
> +                            data-l10n-id="page-source-cmd"
> +                            observes="devtoolsMenuBroadcaster_PageSource"/>

This is an interesting scenario.  The broadcaster manages the value of the 'label' attribute but so does the page-source-cmd message!  I'm not sure it that's bad or not.

::: browser/locales/en-US/chrome/browser/browser.en-US.ftl
@@ +9,5 @@
> +
> +[[ File menu ]]
> +
> +file-menu =
> +    [label]     File

I'm considering using "xul" as the namespace for these traits.  They are after all XUL-specific.  The XULLocalization in l20n.js can be configured to only recognize the "xul" namespace, while "html" is reserved for HTMLLocalization.

@@ +14,5 @@
> +    [accesskey] F
> +tab-cmd =
> +    [label]      New Tab
> +    [accesskey]  T
> +    [key]        t

This works because the whitelist forbids to set the 'key' attribute on <menuitem> nodes and it forbids 'label' and 'accesskey' on <key> nodes.  In other words, tab-cmd is used to localized to distinct elements.  They are still related because the <menuitem> binds to the <key> via the 'key' attribute.

::: toolkit/content/l20n-chrome-xul.js
@@ +580,5 @@
>  
>  const allowed = {
>    attributes: {
>      global: ['aria-label', 'aria-valuetext', 'aria-moz-hint'],
> +    broadcaster: ['bookmarklabel', 'editlabel', 'label', 'sidebartitle'],

I'm not overly happy about adding all of these attributes here but we don't have any other way right now.
Comment on attachment 8773822 [details] [diff] [review]
Patch 1.  browser-menu.inc ported to FTL

Review of attachment 8773822 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/browser.en-US.ftl
@@ +180,5 @@
> +    [key]        +
> +full-zoom-enlarge-cmd-alt1 =
> +    [key]        =
> +full-zoom-enlarge-cmd-alt2 =
> +    [key]        ""

Some commands have one or even two alternative commandkeys bound to them.  Is there a better way to support them than simply introducing additional message like I did here?
I'm not yet really sure if the overlay of command keys is a bug or a feature. It does with a little bit less typing. But it also feels more brittle, and harder to ensure.

An additional feature of having keys always be separate would be that that'd be a consistent pattern that tools could more consistently encourage to not localize.

Looking at the resulting ftl, it's pretty tough with all those attributes. I guess we all kinda thought that attributes would be the edgecase, not the xul-normal?

In terms of files, does it make sense to start getting a bit more modular than just browser.ftl? What if we'd put these into menubar.ft?

I also think we should start to put content into more forward facing locations. I think landing the 'pl' test files in browser/locales/pl/** would be good. Maybe browser/locales/AB_CD/browser/menubar.ftl ? or browser/browser.ftl?
Thanks, Axel, you make good points.

(In reply to Axel Hecht [:Pike] from comment #4)
> I'm not yet really sure if the overlay of command keys is a bug or a
> feature. It does with a little bit less typing. But it also feels more
> brittle, and harder to ensure.

What I like about it is that it's easier to understand and it keeps all relevant translations under a single object -- the entity.  What I dislike about it is that until now I'd been thinking about the DOM whitelist as a security mechanism while here it becomes something more.  Perhaps that's okay; just need to switch my mindset a bit.

> 
> An additional feature of having keys always be separate would be that that'd
> be a consistent pattern that tools could more consistently encourage to not
> localize.

I assume [xul/key] could be handled by tools just as easily?

> Looking at the resulting ftl, it's pretty tough with all those attributes. I
> guess we all kinda thought that attributes would be the edgecase, not the
> xul-normal?

That's my impression as well.  An alternative would be to treat label as the default property to write to rather than textContent.  But that would make other XUL files unhappy.  For instance aboutDialog.xul makes heavy use of <label> and <description> elements and benefits from L20n writing to textContent.

When there's no clear benefit in either of options I tend to lean towards the solution which is internally more coherent.  The current FTL syntax meets this criterion for me: label is after all a proper XUL attribute and it makes sense to have it translated as [xul/label].

I also filed bug 1288443 to make the fallback story better for all those labels.

That said there might be some room for improvement in the current syntax still.  I started to consider suggesting an alternative syntax for defining traits:

  foo-bar[xul/label]     = Foo Bar
  foo-bar[xul/accesskey] = F

…compared to the current syntax:

  foo-bar =
      [xul/label]     Foo Bar
      [xul/accesskey] F

There's a risk of entities being suddenly defined in multiple places or even files.  OTOH, it makes it very obvious what the syntax for referring to these traits from other translations is (foo-bar[xul/label]).  And it's harder to confuse with select expression's variants.  In the current syntax the following translation can be particularly puzzling (but it's an edge-case):

  foo-bar = { $num ->
      [one]   One
     *[other] Many
  }
      [trait] Trait


> In terms of files, does it make sense to start getting a bit more modular
> than just browser.ftl? What if we'd put these into menubar.ft?

Good call, I'll make this change.

> I also think we should start to put content into more forward facing
> locations. I think landing the 'pl' test files in browser/locales/pl/**
> would be good. Maybe browser/locales/AB_CD/browser/menubar.ftl ? or
> browser/browser.ftl?

browser/locales/AB_CD/browser/menubar.ftl sound good.  Is it okay that the new browser/ dir will live next to chrome/ and others?
Flags: needinfo?(l10n)
Blocks: 1289530
I did a quick search for <key> IDs, and most are used only once or not at all, i.e., no UI element exposed how to use the key.

There is at least one though, which is used both from the menubar and the toolbar, https://dxr.mozilla.org/mozilla-central/search?q=key_openDownloads&redirect=false. At least once in the sense that I stopped searching further. For reference, the IDs I went through are the main keyset IDs from browser-sets.inc:

key_newNavigator key_newNavigatorTab focusURLBar focusURLBar2 key_search key_openDownloads key_openAddons openFileKb key_savePage printKb key_close key_closeWindow key_toggleMute key_undo key_redo key_cut key_copy key_paste key_delete key_selectAll goBackKb goForwardKb goHome showAllHistoryKb key_fullScreen key_reload key_viewSource key_viewInfo key_find key_findAgain key_findPrevious addBookmarkAsKb bookmarkAllTabsKb manBookmarkKb viewBookmarksSidebarKb markPage focusChatBar key_stop key_gotoHistory key_fullZoomReduce key_fullZoomEnlarge key_fullZoomReset key_showAllTabs key_switchTextDirection key_privatebrowsing key_sanitize key_undoCloseWindow key_selectTab1 key_selectTab2 key_selectTab3 key_selectTab4 key_selectTab5 key_selectTab6 key_selectTab7 key_selectTab8 key_selectLastTab


As for your syntax question, Dwayne has been voicing a strong opinion on how just a single key and a complex value are so much better for tooling, and I think I'd also like it to stay that way.
Flags: needinfo?(l10n)
Attached patch Patch 2 (obsolete) (deleted) — — Splinter Review 
Thanks, Pike, I applied your feedback:  the command keys are now localized in separate entries.  I also split browser.ftl into menubar.ftl and toolbar.ftl.

With Pike's help this patch moves en-US FTL files into resource://chrome/en-US/browser/* and resource://gre/chrome/en-US/global.  I also removed all the Polish translations which we used for testing.  We need to implement a proper build support to make them available under resource://chrome/pl/*.

The patch can also be seen at:

    https://github.com/stasm/gecko-dev/compare/l20n...l20n-1288406
Attachment #8773822 - Attachment is obsolete: true
Attachment #8773822 - Flags: feedback?(l10n)
Attachment #8773822 - Flags: feedback?(gandalf)
Attachment #8775249 - Flags: feedback?(l10n)
Attachment #8775249 - Flags: feedback?(gandalf)
Comment on attachment 8775249 [details] [diff] [review]
Patch 2

I meant to ask for reviews.
Attachment #8775249 - Flags: review?(l10n)
Attachment #8775249 - Flags: review?(gandalf)
Attachment #8775249 - Flags: feedback?(l10n)
Attachment #8775249 - Flags: feedback?(gandalf)
Comment on attachment 8775249 [details] [diff] [review]
Patch 2

Review of attachment 8775249 [details] [diff] [review]:
-----------------------------------------------------------------

I recall that you also wanted to drop the -cmd suffixes on the menuitem IDs?

I found a few nits in the strings, and one part where the original code does something fancy between redo and undo? Wonky. Try to figure out the backstory on this one?

With that, r=me.

::: browser/base/content/browser-sets.inc
@@ +242,1 @@
>  #endif

This bugger I find wonky?

::: browser/locales/en-US/browser/menubar.ftl
@@ +73,5 @@
> +    [xul/label]      Work Offline
> +    [xul/accesskey]  k
> +
> +quit-application-cmd =
> +    [lxul/abel]      { OS() ->

[xul/label]

(hah, the syntax can't be all that bad)

@@ +218,5 @@
> +page-style-persistent-only =
> +    [xul/label]      Basic Page Style
> +    [xul/accesskey]  b
> +
> +# These should match what Safari and other Apple applications use on OS X Lion.

"These" is not qualified. Use a section and section comment?

@@ +256,5 @@
> +history-restore-last-session =
> +    [xul/label]        Restore Previous Session
> +history-undo-menu =
> +    [xul/label]        Recently Closed Tabs
> +# See bug 394759

Remove, this used to be a pre-landed string, and is no more.

@@ +271,5 @@
> +    [xul/label]      Show All Bookmarks
> +bookmark-this-page-cmd =
> +    [xul/label]          Bookmark This Page
> +    [xul/bookmarklabel]  Bookmark This Page
> +    [xul/editlabel]      Edit This Page

should we have a comment about the difference between label, editlabel, and bookmarklabel?

::: browser/locales/en-US/browser/toolbar.ftl
@@ +2,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +urlbar =
> +    [xul/placeholder] Search or enter address (FTL)

Drop the FTL now?
Attachment #8775249 - Flags: review?(l10n) → review+
(In reply to Axel Hecht [:Pike] from comment #9)

> I recall that you also wanted to drop the -cmd suffixes on the menuitem IDs?

I dropped them from -keys, so foo-cmd-key became simply foo-key.  Are you suggesting foo-cmd → foo?

> ::: browser/base/content/browser-sets.inc
> @@ +242,1 @@
> >  #endif
> 
> This bugger I find wonky?

I think this is due to how Linux vs. other OS handle redo.  In Windows and Max there's a different command key for redo (e.g. Ctrl+Y).  On Linux you're supposed to use the same key as for Undo but with an additional accelerator: Shift. 

> @@ +271,5 @@
> > +    [xul/label]      Show All Bookmarks
> > +bookmark-this-page-cmd =
> > +    [xul/label]          Bookmark This Page
> > +    [xul/bookmarklabel]  Bookmark This Page
> > +    [xul/editlabel]      Edit This Page
> 
> should we have a comment about the difference between label, editlabel, and
> bookmarklabel?

Yeah, if only I knew what it was :)  They're all used by a broadcaster but I don't know how.

I applied the rest of your feedback on my branch, thanks!
Comment on attachment 8775249 [details] [diff] [review]
Patch 2

lgtm.

I only question if we should use "xul" namespace. I think I prefer "dom" or "html" for all DOM-related attributes, but we can sort it out later.
Attachment #8775249 - Flags: review?(gandalf) → review+
re bookmark and edit label, that's actually https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#1667.

I guess we should rewrite that in l20n?
Review commit: https://reviewboard.mozilla.org/r/68008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68008/
Attachment #8776091 - Flags: review?(gandalf)
Attachment #8776092 - Flags: review?(l10n)
Attachment #8776093 - Flags: review?(l10n)
Attachment #8776093 - Flags: review?(gandalf)
Add menubar.ftl and toolbar.ftl required to localize browser's visible UI and
expose them under resource://chrome/en-US/browser/.

Review commit: https://reviewboard.mozilla.org/r/68010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68010/
Comment on attachment 8775249 [details] [diff] [review]
Patch 2

I rebased the patch on top of larch and split them into smaller changes.  I also applied Pike's feedback regarding identifier names and the bookmarklabel/editlabel logic (now done via setting data-l10n-id on the broadcaster).
Attachment #8775249 - Attachment is obsolete: true
Comment on attachment 8776091 [details]
Bug 1288406 - Part 1: Add l20n-chrome-xul.js.

assuming we're willing to land code that we know will have to be refactored to remove the performance issues that it causes, I'm ok with landing it.
Attachment #8776091 - Flags: review?(gandalf) → review+
Comment on attachment 8776093 [details]
Bug 1288406 - Part 3: Port browser's menubar to L20n.

https://reviewboard.mozilla.org/r/68012/#review65190

::: browser/base/content/browser.xul:79
(Diff revision 1)
>  <script type="application/javascript" src="chrome://browser/content/downloads/indicator.js"/>
>  <script type="application/javascript" src="chrome://browser/content/places/editBookmarkOverlay.js"/>
>  
> +<link rel="localization" href="/browser/menubar.ftl"/>
> +<link rel="localization" href="/browser/toolbar.ftl"/>
> +<script type="application/javascript" src="chrome://global/content/l20n-chrome-xul.js"/>

can you put scripts above links? This will allow us to rework the logic in bindings to pick up links as we parse.
Attachment #8776093 - Flags: review?(gandalf) → review+
I'm just waking up to the revelation that xul:broadcaster might actually broadcast data-l10n-id. I think we have use-cases where both the broadcaster and the observing element specify a data-l10n-id, can you check if that breaks?
Flags: needinfo?(stas)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17)

> assuming we're willing to land code that we know will have to be refactored
> to remove the performance issues that it causes, I'm ok with landing it.

Yes, I filed bug 1289535 specifically to address this.
(In reply to Axel Hecht [:Pike] from comment #19)
> I'm just waking up to the revelation that xul:broadcaster might actually
> broadcast data-l10n-id. I think we have use-cases where both the broadcaster
> and the observing element specify a data-l10n-id, can you check if that
> breaks?

I'm having some trouble testing it right now but it looks like you're right.  This doesn't really break per se, but might not be optimal.  First off, we could remove data-l10n-ids from the observers as they don't seem needed anymore.  If I understand the docs correctly, <broadcaster> broadcasts its data-l10n-id to the obervers, then L20n's mutation observer kicks in and localizes both the broadcaster and the observer, and then the broadcaster boadcasts the values of translated attributes again.

This back-and-forth could be avoided if instead of <menuitem observes"..."> we used an explicit <observes> element as a child of the menuitem.  It would allow us to specify which attribute defined on the broadcaster to observe.  We could then either choose to only broadcast data-l10n-ids (relying on the mutation observer) or to only broadcast the translated "label" attribute.

I'll fix my local setup and report back with confirmation once I'm able to test it.
Flags: needinfo?(stas)
IIRC, we're having the following pattern somewhere in our code:

<broadcaster id='foopy' label='smokin'/>

<menuitem id='one_menu' observes='foopy' accesskey='o'/>
<menuitem id='other_menu' observes='foopy' accesskey='p'/>

In that scenario, we couldn't separate out the accesskey in one place from the accesskey in another :-/

I did contemplate putting a data-l10n-data? into the observer, and then to format the label depending on "show" or "edit", but that's also gross.

Or we can ask to investigate if there are already special attrs that don't get broadcasted, and tie into that. Or hack that.
(In reply to Axel Hecht [:Pike] from comment #22)
> IIRC, we're having the following pattern somewhere in our code:
> 
> <broadcaster id='foopy' label='smokin'/>
> 
> <menuitem id='one_menu' observes='foopy' accesskey='o'/>
> <menuitem id='other_menu' observes='foopy' accesskey='p'/>
> 
> In that scenario, we couldn't separate out the accesskey in one place from
> the accesskey in another :-/
> 
> I did contemplate putting a data-l10n-data? into the observer, and then to
> format the label depending on "show" or "edit", but that's also gross.
> 
> Or we can ask to investigate if there are already special attrs that don't
> get broadcasted, and tie into that. Or hack that.

I'm basing the following on https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Broadcasters_and_Observers.

All attributes are broadcast except for 'id' and 'persist'.  If we want to get more granular we could use the <observes> element.  Here's how I understand it (using your example above):

    <broadcaster id='foopy' label='smokin'/>

    <menuitem id='one_menu' accesskey='o'>
        <observes element="foopy" attribute="label"/>
    </menuitem>
    <menuitem id='other_menu' accesskey='p'>
        <observes element="foopy" attribute="label"/>
    </menuitem>

I haven't tested it but I'll be happy to give it a try if you think that's a good idea.
Ah, yeah, thanks for finding the docs here. I think it'd be a good idea to be very specific about including attributes from localized broadcasters, and maybe not try to sanitize localized attributes on broadcaster themselves?
Before this change the <broadcaster> elements would broadcast their
data-l10n-ids to the obervers and the L20n's mutation observer would kick in
and localize both the broadcaster and the observer.  After having their
localizable attributes like `label` localized the broadcasters would boadcast
them to the observes again.

By using the <observes> elements inside of <menuitem> elements we can be
specific about which attributes are observed.

By not broadcasting data-l10n-ids it will be possible in the future to add them
specifically to the observing elements to define their own accesskeys not tied
to the global broadcaster.

Review commit: https://reviewboard.mozilla.org/r/68514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68514/
Attachment #8776091 - Attachment description: Add l20n-chrome-xul.js → Bug 1288406 - Part 1: Add l20n-chrome-xul.js.
Attachment #8776092 - Attachment description: Add {menubar,toolbar}.ftl files → Bug 1288406 - Part 2: Add {menubar,toolbar}.ftl files.
Attachment #8776093 - Attachment description: Bug 1288406 - Port browser's menubar to L20n → Bug 1288406 - Part 3: Port browser's menubar to L20n.
Attachment #8776847 - Flags: review?(l10n)
Attachment #8776091 - Flags: review+ → review?(gandalf)
Comment on attachment 8776091 [details]
Bug 1288406 - Part 1: Add l20n-chrome-xul.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68008/diff/1-2/
Comment on attachment 8776092 [details]
Bug 1288406 - Part 2: Add {menubar,toolbar}.ftl files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68010/diff/1-2/
Comment on attachment 8776093 [details]
Bug 1288406 - Part 3: Port browser's menubar to L20n.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68012/diff/1-2/
(In reply to Axel Hecht [:Pike] from comment #24)
> Ah, yeah, thanks for finding the docs here. I think it'd be a good idea to
> be very specific about including attributes from localized broadcasters, and
> maybe not try to sanitize localized attributes on broadcaster themselves?

I explicitly listed accesskey and label in the whitelist for broadcasters for now.  Let's see if we need more attributes going forward.  I'd prefer to not allow all -- just in case developers don't use <observes> elements in their code.

Related to this:  should accesskey and label be allowed to be localized on all XUL elements?  Right now I define them as such separately for every element (broadcaster, button, menuitem, menu, tab etc.) but perhaps this can be simplified by defining them as allowed for all elements.
Flags: needinfo?(l10n)
I've asked for a try build, mostly because I'm concerned about all the non-localized attributes on the sidebar broadcasters.

Re whitelisting label/accesskey, it seems that there are various ways to denote that a thing has label and accesskey, I found at least nsIDOMXULSelectCntrlItemElement and nsIDOMXULLabeledControlElement. Obviously the first doesn't inherit from the latter, even though it's a superset. Doh XUL.

I'm fine with just whitelisting, I'd despise anyone that tried to put something not-xul-accesskey on those attributes.
Flags: needinfo?(l10n)
I'm having trouble debugging the mochitest failures on try.  I can't reproduce them on my own Linux64 opt builds.  There are also many failures on Mac builds and I suspect I might have broken some accesskey using our OS() -> ... syntax.
Mass change dependency tree for bug 1279002 into a whiteboard keyword.
No longer blocks: gecko-l20n
Whiteboard: [gecko-l20n]
I just fired up the mac build from try, and its UI is pretty broken.

Browser console complains:

ReferenceError: Unknown entity: brand-shorter-name
Stack trace:
EntityReference@resource://gre/modules/IntlMessageContext.jsm:946:16
Value@resource://gre/modules/IntlMessageContext.jsm:1025:26
Pattern@resource://gre/modules/IntlMessageContext.jsm:1127:16
Value@resource://gre/modules/IntlMessageContext.jsm:1010:19
Value@resource://gre/modules/IntlMessageContext.jsm:1032:21
Pattern@resource://gre/modules/IntlMessageContext.jsm:1127:16
Blocks: 1291693
After talking to stas, we have enough XUL support for us to work in this, unblocking from bug 1280669.
No longer depends on: 1280669
The Quit menu item on Mac OS X needs brand-shorter-name to be defined.

Review commit: https://reviewboard.mozilla.org/r/68968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68968/
Attachment #8777389 - Flags: review?(l10n)
Comment on attachment 8776092 [details]
Bug 1288406 - Part 2: Add {menubar,toolbar}.ftl files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68010/diff/2-3/
Comment on attachment 8776093 [details]
Bug 1288406 - Part 3: Port browser's menubar to L20n.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68012/diff/2-3/
Comment on attachment 8776847 [details]
Bug 1288406 - Part 4: Explicitly observe broadcasters' localizable attributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68514/diff/1-2/
Attached image File menu without Quit Firefox (deleted) — 
I'm not sure what's going on here, but the Quit menu doesn't show, and its commandkey isn't working either.

There's a hidden="true" attribute on it, which surprises me, but I only dug into the chrome debugger a little bit.

I didn't find anything in the browser console this time.
Comment on attachment 8776847 [details]
Bug 1288406 - Part 4: Explicitly observe broadcasters' localizable attributes.

https://reviewboard.mozilla.org/r/68514/#review66044

I like the general idea of this step, but it breaks the sidebars for history and bookmarks, at least, probably because we only carry over the label, and not the rest.

Maybe in this case, keep using the <observes> element, but use * as attribute with an explicit comment?
Attachment #8776847 - Flags: review?(l10n) → review-
Attachment #8776092 - Flags: review?(l10n) → review+
Comment on attachment 8776092 [details]
Bug 1288406 - Part 2: Add {menubar,toolbar}.ftl files.

https://reviewboard.mozilla.org/r/68010/#review66046

r=me with the branding ftl patch as part of this.
Comment on attachment 8777389 [details]
Bug 1288406 - Part 5: Port branding to FTL.

https://reviewboard.mozilla.org/r/68968/#review66048
Attachment #8777389 - Flags: review?(l10n) → review+
https://reviewboard.mozilla.org/r/68012/#review66050

I'd like to keep the r? open on this one until we figure out what's wrong the quit menu/commandkey on mac.
When Firefox is not focused, but owning the OS/X menubar, I see only part of the menus. This is how this looks.
Using L20n's OS() function doesn't currently work with XUL's "key" attributes.
The key shortcuts become inactive even though the "key" attributes are
localized and set correctly.  I filed bug 1291915 to track this issue.

Review commit: https://reviewboard.mozilla.org/r/69124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69124/
Attachment #8777591 - Flags: review?(l10n)
Attachment #8776847 - Flags: review- → review?(l10n)
Comment on attachment 8776092 [details]
Bug 1288406 - Part 2: Add {menubar,toolbar}.ftl files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68010/diff/3-4/
Comment on attachment 8776093 [details]
Bug 1288406 - Part 3: Port browser's menubar to L20n.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68012/diff/3-4/
Comment on attachment 8776847 [details]
Bug 1288406 - Part 4: Explicitly observe broadcasters' localizable attributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68514/diff/2-3/
Comment on attachment 8777389 [details]
Bug 1288406 - Part 5: Port branding to FTL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68968/diff/1-2/
Comment on attachment 8777591 [details]
Bug 1288406 - Part 6: Don't use OS() for platform-specific key shortcuts for now.

https://reviewboard.mozilla.org/r/69124/#review66208

With the copy-nit below, r=me, assuming the build works.

::: browser/locales/en-US/browser/menubar.ftl:80
(Diff revision 1)
>  
>  quit-application-menuitem =
> -    [xul/label]      { OS() ->
> -        [win]    Exit
> -        [mac]    Quit { brand-shorter-name }
> -       *[lin]    Quit
> +    [xul/label]      Quit
> +    [xul/accesskey]  Q
> +quit-application-menuitem-win =
> +    [xul/label]      Exit Offline

I think the "Offline" slipped in here?
Attachment #8777591 - Flags: review?(l10n) → review+
Attachment #8776847 - Flags: review?(l10n) → review+
Comment on attachment 8776847 [details]
Bug 1288406 - Part 4: Explicitly observe broadcasters' localizable attributes.

https://reviewboard.mozilla.org/r/68514/#review66210

Assuming the explicit observes fix the sidebar, r=me
Comment on attachment 8776093 [details]
Bug 1288406 - Part 3: Port browser's menubar to L20n.

https://reviewboard.mozilla.org/r/68012/#review66214
Attachment #8776093 - Flags: review?(l10n) → review+
https://reviewboard.mozilla.org/r/68012/#review65190

> can you put scripts above links? This will allow us to rework the logic in bindings to pick up links as we parse.

I'm going to keep them as they are right now.  We'll likely experiment with different positioning as we do more perf testing anyways.  See bug 1289535 comment 2.
Comment on attachment 8776093 [details]
Bug 1288406 - Part 3: Port browser's menubar to L20n.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68012/diff/4-5/
Comment on attachment 8776847 [details]
Bug 1288406 - Part 4: Explicitly observe broadcasters' localizable attributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68514/diff/3-4/
Comment on attachment 8777389 [details]
Bug 1288406 - Part 5: Port branding to FTL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68968/diff/2-3/
Comment on attachment 8777591 [details]
Bug 1288406 - Part 6: Don't use OS() for platform-specific key shortcuts for now.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69124/diff/1-2/
(In reply to Staś Małolepszy :stas from comment #61)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=37963736ff85

The try run looks good with a few intermittents and a consistent failure in bc7 on Linux 64 builds.  I filed bug 1292128 to track this.

(In reply to Staś Małolepszy :stas from comment #60)
> Comment on attachment 8777591 [details]
> Bug 1288406 - Part 6: Don't use OS() for platform-specific key shortcuts for now.

Bug 1292127.
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1292128
I don't know why `hg push` cut off the last changeset from the previous comment.

http://hg.mozilla.org/projects/larch/rev/37963736ff85
Bug 1288406 - Part 6: Don't use OS() for platform-specific key shortcuts for now. r=Pike
This is now fixed.

\o/
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #8776091 - Flags: review?(gandalf) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: