867543 - Change - Context app bar buttons should have labels

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: App Bar, defect, P2)

Description

[Sam Foster [:sfoster] (he/him)]

Mockups show text labels below the icons for the context app bar buttons. This helps disambiguate the icons. Label text will need to be localized.

See: https://bug808770.bugzilla.mozilla.org/attachment.cgi?id=679763
..and another mockup which I'm so far unable to locate.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Visual mockup of contextual actions for top sites tile selections: 
https://bug836387.bugzilla.mozilla.org/attachment.cgi?id=726419

Comment 3

[Jonathan Wilde [:jwilde]]

Attachment: part1 - generalize appbar css, make sprites more robust, move into platform

Comment 4

[Jonathan Wilde [:jwilde]]

Attachment: part1 - generalize appbar css, make sprites more robust, move into platform

Previous patch was the wrong one. This patch does some cleanup that'll make future work on this and the navbar somewhat easier and less fragile.

Comment 5

[Jonathan Wilde [:jwilde]]

p=5

Comment 6

[Jonathan Wilde [:jwilde]]

Attachment: part2 - adding styling for labels below buttons on the appbar

Comment 7

[Jonathan Wilde [:jwilde]]

Comment on attachment 776095 [details] [diff] [review]
part2 - adding styling for labels below buttons on the appbar

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

::: browser/metro/base/content/browser.xul
@@ +290,4 @@
>              <toolbarbutton id="stop-button" command="cmd_stop"/>
>            </hbox>
>  
> +          <toolbarbutton id="download-button" class="appbar-secondary" oncommand="Appbar.onDownloadButton()"/>

This hunk should've been in the part1 patch.

::: browser/metro/theme/platform.css
@@ +684,5 @@
> +  /* Slow the bottom up transition since it's impossible to match the system's
> +     soft keyboard movement. */
> +  transition: transform @metro_animation_duration@ @metro_animation_easing@,
> +              bottom @appbar_keyboard_slideup_duration@ @metro_animation_easing@;
> +}

Same for this.

Comment 8

[Jonathan Wilde [:jwilde]]

Attachment: part1 - generalize appbar css, make sprites more robust, move into platform

Comment 9

[Jonathan Wilde [:jwilde]]

Attachment: part2 - adding styling for labels below buttons on the appbar

Comment 10

[Jonathan Wilde [:jwilde]]

Attachment: part3 - implement labels, with correct pluralization and locale support

Comment 11

[Jonathan Wilde [:jwilde]]

Attachment: part3 - implement labels, with correct pluralization and locale support

Refining naming of the Map instance.

Comment 12

[Matt Brubeck (:mbrubeck)]

Comment on attachment 776143 [details] [diff] [review]
part2 - adding styling for labels below buttons on the appbar

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

::: browser/metro/theme/browser.css
@@ +415,5 @@
>    transition-delay: 80ms;
>  }
> +#contextualactions-tray > toolbarbutton > .toolbarbutton-text {
> +  color: white;
> +}

Can you just do #contextualactions-tray { color: white; } and let the button text inherit it?

::: browser/metro/theme/platform.css
@@ +497,4 @@
>    /* Make the link take up all the space before the buttons. */
>    -moz-box-flex: 9999;
>  }
> +appbar toolbar[labelled=true] {

optional style nit: I prefer to use just "toolbar[labelled]" for boolean attributes (just for aesthetics and conciseness).

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 776147 [details] [diff] [review]
part3 - implement labels, with correct pluralization and locale support

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

::: browser/metro/base/content/TopSites.js
@@ +247,5 @@
>          // fire a MozContextActionsChange event to update the context appbar
>          let event = document.createEvent("Events");
>          event.actions = [...nextContextActions];
> +        event.noun = tileGroup.contextNoun;
> +        event.qty = selectedTiles.length;

Could the event listener in appbar.js retrieve these from the event target (the <richgrid>) rather than having each event sender do it?  I guess we'd still need special logic for the quantity of deleted/hidden items somewhere.

Maybe the better way to reduce code duplication in the long run is to share more code between the various grid controllers.

Comment 14

[Sam Foster [:sfoster] (he/him)]

Yeah I think the idea was always that we would be able to refactor the grid controllers at the point when we know more about what common functionality they needed to share. 
I'm in this same bit of code while working on bug 876033, where the undo delete functionality will need to be shared in some way across all the controllers eventually. Maybe creating a simple View base class or prototype which each of the *Views inherit from will give us something to build on and tackle this incrementally.

Comment 15

[Jonathan Wilde [:jwilde]]

(In reply to Matt Brubeck (:mbrubeck) from comment #12)
> Comment on attachment 776143 [details] [diff] [review]
> part2 - adding styling for labels below buttons on the appbar
> 
> Review of attachment 776143 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/theme/browser.css
> @@ +415,5 @@
> >    transition-delay: 80ms;
> >  }
> > +#contextualactions-tray > toolbarbutton > .toolbarbutton-text {
> > +  color: white;
> > +}
> 
> Can you just do #contextualactions-tray { color: white; } and let the button
> text inherit it?

Not without !important, but putting color: white; in #contextualactions-tray > toolbarbutton works. I'd assume that the latter is preferable since it lets us drop a selector, but also prevent the rapid expansion of the !important?

Comment 16

[Matt Brubeck (:mbrubeck)]

(In reply to Jonathan Wilde [:jwilde] from comment #15)
> > Can you just do #contextualactions-tray { color: white; } and let the button
> > text inherit it?
> 
> Not without !important, but putting color: white; in #contextualactions-tray
> > toolbarbutton works. I'd assume that the latter is preferable since it
> lets us drop a selector, but also prevent the rapid expansion of the
> !important?

Ah, I see.  In that case, I'd stick with your original .toolbarbutton-text selector.  The class selector has potential performance benefits over "toolbarbutton", and expresses the intent nicely.

Comment 17

[Jonathan Wilde [:jwilde]]

(In reply to Sam Foster [:sfoster] from comment #14)
> Yeah I think the idea was always that we would be able to refactor the grid
> controllers at the point when we know more about what common functionality
> they needed to share. 
> I'm in this same bit of code while working on bug 876033, where the undo
> delete functionality will need to be shared in some way across all the
> controllers eventually. Maybe creating a simple View base class or prototype
> which each of the *Views inherit from will give us something to build on and
> tackle this incrementally.

So, in terms of plan of action on this bug, should we land as-is and file a follow-up refactoring bug to make a generic *View prototype?

Comment 18

[Matt Brubeck (:mbrubeck)]

(In reply to Jonathan Wilde [:jwilde] from comment #17)
> So, in terms of plan of action on this bug, should we land as-is and file a
> follow-up refactoring bug to make a generic *View prototype?

Yes, feel free to land as-is.

Comment 19

[Jonathan Wilde [:jwilde]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1a8bea0943
https://hg.mozilla.org/integration/mozilla-inbound/rev/c237d7e7230e
https://hg.mozilla.org/integration/mozilla-inbound/rev/47fce66eae89

Comment 20

[Francesco Lodolo [:flod]]

Hi,
about plural forms
https://hg.mozilla.org/integration/mozilla-inbound/diff/ae1a8bea0943/browser/metro/locales/en-US/chrome/browser.properties

Sadly some of our tools (compare-locales) and some external tools used for localization rely on the presence of this comment to identify a string with plural form.

# LOCALIZATION NOTE (STRINGID): Semi-colon list of plural forms.
# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals

The bad part is that his comment must be added before each string using a plural form.

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/47fce66eae89
https://hg.mozilla.org/mozilla-central/rev/c237d7e7230e
https://hg.mozilla.org/mozilla-central/rev/ae1a8bea0943

Comment 22

[Jonathan Wilde [:jwilde]]

(In reply to Francesco Lodolo [:flod] from comment #20)
> Hi,
> about plural forms
> https://hg.mozilla.org/integration/mozilla-inbound/diff/ae1a8bea0943/browser/
> metro/locales/en-US/chrome/browser.properties
> 
> Sadly some of our tools (compare-locales) and some external tools used for
> localization rely on the presence of this comment to identify a string with
> plural form.
> 
> # LOCALIZATION NOTE (STRINGID): Semi-colon list of plural forms.
> # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> 
> The bad part is that his comment must be added before each string using a
> plural form.

Thank you! :D Filed a followup bug 894949.

Comment 23

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130806104538
Built from http://hg.mozilla.org/mozilla-central/rev/1e381c91885d

WFM
Tested on windows 8 using latest nightly. I see text labels below every icons

Comment 24

[Samvedana (:Samvedana)]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130815030203
Built from http://hg.mozilla.org/mozilla-central/rev/a8daa428ccbc

WFM
Tested on windows 8 using latest nightly for iteration-12. I see text labels below every icons.