811835 - Customizing toolbar to show "Text" or "Icons & Text" breaks the Social toolbar UI

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Ibrahima SARR]

Attachment: funny_ff_api_icons.PNG

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20121106195758

Steps to reproduce:

Enabled SocialApi on Firefox 17 beta.


Actual results:

Facebook Icons look blurry and stretched


Expected results:

Just normal icons that display on Facebook pages

Comment 1

[Virgil Dicu [:virgil] [QA]]

Are you using any add-ons/themes that might cause this? Does it reproduce in Safe mode or with a clean profile?

Comment 2

[Ibrahima SARR]

No, I am not using a theme. And if I use safe mode, the SocialApi does not work.

I found out what the problem is: You only get the icons displaying correctly when you select "icons only" in "Customize Toolbar" Window. Selecting "icon & text" causes this issue. Apparently, SocialApi icons don't have text associated to them.

Comment 3

[u279076]

Nice find Ibrahima. This is definitely reproducible for me now.

Icons Only > Facebook toolbar displays as expected
Icons & Text > Facebook toolbar appears with large, stretched buttons
Text Only > Facebook toolbar disappears

Comment 4

[Ibrahima SARR]

Exactly! Thanks for updating the title of the bug.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Early WIP patch

This is the early WIP patch that I started on, but I've been thinking about this a bit and would like to see us switch to using regular xul:toolbarbuttons and figuring out a different solution for the notification count overlay.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP patch

This patch is close but doesn't support the notification counts yet. I also need to update the tests and will eventually remove all the unneeded styles.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

The tests pass with this patch, but the positioning of the notification icon could use some tweaking, and it is currently hidden in Text & Icons mode until I can get the positioning better.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v.2

This patch is very close with the notification counts now appearing in their correct location in the top right of each toolbar button.

Issues with this patch:
1) In order to get the positioning of the notification counts in the correct corner, I had to use a hack that changed the CSS direction of each toolbar element to be opposite of the native direction (e.g. LTR->RTL).
2) Issue 1 means that ::before now appears at the end of an element. ::before was used to create the separator of the combo toolbar buttons. I turned off this separator to make it not appear as bad. Maybe we're OK without the button separators? The separator could use ::after, and the notification could could use ::before, but there is still issue 4.
3) If we keep the separators, then the toolbar buttons flicker when being hovered over them. I'm pretty sure this has to do with the buttons overlapping and causing an issue in the layout engine. I've played with the CSS properties around this and have been unable to remove the overlapping of the buttons (although I think if [A] was solved this might be simple).
4) Still need to tweak styling for gnomestripe and pinstripe.

Things that would have made this patch easier to write:
A) The generated content for the notification count is supposed to act as a child element of the associated element. Therefore, it should be possible to set |position:relative| on each social toolbar button, and then set |position:absolute| on the generated content. This would have made positioning the notification count in the corner trivial, but for some reason it is not behaving as it should. This is the cause for my hack of changing the direction and not using CSS left or CSS right properties (and just using relative adjustments via -moz-margin-start).

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v.3

I was able to remove the direction:rtl hack by introducing another toolbaritem surrounding the toolbarbutton. Matt and Mike, can either of you take a look at this?

I need test this on Linux and Mac and make whatever visual tweaks that are necessary there.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v.3.1

With proper test adjustments.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 693601 [details] [diff] [review]
Patch v.3.1

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

So, I'm not *too* wild about setting content in CSS to add the notification to the toolbar buttons... It's a non-obvious solution that mixes presentation with content.

I'm trying to think of a better option...

The only thing that comes to mind is a new binding that extends toolbarbutton, which allows badges. If notificationCount != "", attach the binding, and display the notification on top of the icon image via <stack>.

That's my first gut instinct though, and I really don't know what constraints you're under, so take it with a grain of salt.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1

I feel that this patch is now ready for review. There exists one remaining issue, which is that the toolbar buttons are a little too wide on OS X. Any help here would be appreciated.

I tested this patch on Windows, OS X, and Ubuntu.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike Conley (:mconley) from comment #11)
> The only thing that comes to mind is a new binding that extends
> toolbarbutton, which allows badges. If notificationCount != "", attach the
> binding, and display the notification on top of the icon image via <stack>.

(Replied over IRC, but good to leave a reply here for posterity)

The approach previous to this bug uses a custom solution (although not an extension of the binding) to achieve this using a stack. We have quite a few issues with the toolbar buttons and my hope is that the closer we can get to regular toolbarbuttons the better.

As for using generated content here instead of actual elements: If we think about these as notification badges, then I think using pure styling isn't a problem. This is similar to our apptab glow, except that it shows a number.

Comment 14

[Alex Keybl [:akeybl]]

We're now in the timeframe of our final beta - doubt this is severe enough (or has enough impacted users) to fix for FF18 at this point. Please email if we've got that wrong.

Comment 15

[u279076]

(In reply to Alex Keybl [:akeybl] from comment #14)
> We're now in the timeframe of our final beta - doubt this is severe enough
> (or has enough impacted users) to fix for FF18 at this point. Please email
> if we've got that wrong.

I tend to agree. However if the risk is low and the patch comes in time, I'd like to see this land in 18.0b6 or as an 18.0.1 ride-along (if there is one). I think wontfix is the right call, with the caveat that we'll get it in if we can.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 693983 [details] [diff] [review]
Patch v1

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

I didn't test on non-OSX since you did.

::: browser/base/content/browser-social.js
@@ +780,5 @@
>      this.updateButtonHiddenState();
>      let provider = Social.provider;
>      let icons = provider.ambientNotificationIcons;
>      let iconNames = Object.keys(icons);
> +    let socialToolbarItem = document.getElementById("social-toolbar-item");

This function is way too large.  It should be refactored into more manageable pieces in a separate bug.

Please move this definition closer to where it's first used.

@@ +870,5 @@
> +        // not being constrained to the bounding box of a parent toolbarbutton that has position:relative.
> +        toolbarButtonContainer = document.createElement("toolbaritem");
> +        toolbarButtonContainer.classList.add("social-notification-container");
> +        toolbarButtonContainer.setAttribute("id", toolbarButtonContainerId);
> +        toolbarButton = document.createElement("toolbarbutton");

Nit: insert a newline between the container and button code. A comment wouldn't hurt either but is perhaps unnecessary.

@@ +890,4 @@
>  
> +      let badge = icon.counter || "";
> +      if (toolbarButton.getAttribute("badge") != badge)
> +        toolbarButton.setAttribute("badge", badge);

Is this badge text accessible anywhere for screen readers? From what I read, many screen readers don't see generated content because it doesn't exist in the markup.

Also, I don't see any ARIA/a11y code here to notify users of changes.  It seems like ARIA live regions or the XUL equivalent should be used for the badges. I think the labels were readable in the previous implementation so this may cause a regression. I'm fine with a11y of badges being fixed in a follow-up where you can get more input from others.

@@ +943,5 @@
>  
>      panel.addEventListener("popupshown", function onpopupshown() {
>        panel.removeEventListener("popupshown", onpopupshown);
>        aToolbarButtonBox.setAttribute("open", "true");
> +      aToolbarButtonBox.parentNode.setAttribute("open", "true");

Is this attribute actually needed on both elements at this time?

::: browser/base/content/test/social/browser_social_mozSocial_API.js
@@ +27,5 @@
>          triggerIconPanel();
>      }
>  
>      function triggerIconPanel() {
> +      let statusIcon = document.querySelector("#social-toolbar-item > .social-notification-container:nth-child(2) > .toolbarbutton-1");

Why the nth-child in the tests?

::: browser/themes/pinstripe/browser.css
@@ +3790,5 @@
>  }
>  
>  /* === social toolbar button === */
>  
> +toolbar[mode="icons"] .social-notification-container {

How about toolbar[mode="icons"] > #social-toolbar-item > .social-notification-container for improved perf.

@@ +3803,1 @@
>    margin-left: 0;

I think you also want this to match against the jewels which are no longer children of the #social-toolbar-item.  Right now this patch causes an extra 4px of margin on the left and right of the jewels on OS X.

@@ +3820,5 @@
> +  position: relative;
> +}
> +
> +.social-notification-container > .toolbarbutton-1[badge]:not([badge=""])::after {
> +  content: attr(badge);

The "content" property probably belongs in browser/base/ otherwise no badge text will be shown on 3rd party themes unless they know to copy this.

@@ +3825,4 @@
>    font-size: 9px;
>    font-weight: bold;
>    padding: 0 1px;
> +  color: #fff;

Nit: why the change from "white"?

@@ +3843,5 @@
> +toolbar[mode="icons"] .social-notification-container > .toolbarbutton-1[badge]:not([badge=""])::after {
> +  right: -2px;
> +}
> +
> +toolbar[mode="icons"] .social-notification-container > .toolbarbutton-1[badge]:not([badge=""]):-moz-locale-dir(rtl)::after {

Use child selectors with #social-toolbar-item in these two places too.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

(In reply to Matthew N. [:MattN] from comment #16)
> ::: browser/base/content/browser-social.js
> @@ +780,5 @@
> >      this.updateButtonHiddenState();
> >      let provider = Social.provider;
> >      let icons = provider.ambientNotificationIcons;
> >      let iconNames = Object.keys(icons);
> > +    let socialToolbarItem = document.getElementById("social-toolbar-item");
> 
> This function is way too large.  It should be refactored into more
> manageable pieces in a separate bug.

Filed bug 824784.

> Please move this definition closer to where it's first used.

Done.

> @@ +870,5 @@
> > +        // not being constrained to the bounding box of a parent toolbarbutton that has position:relative.
> > +        toolbarButtonContainer = document.createElement("toolbaritem");
> > +        toolbarButtonContainer.classList.add("social-notification-container");
> > +        toolbarButtonContainer.setAttribute("id", toolbarButtonContainerId);
> > +        toolbarButton = document.createElement("toolbarbutton");
> 
> Nit: insert a newline between the container and button code. A comment
> wouldn't hurt either but is perhaps unnecessary.

Added a newline, I'm not sure what a comment would provide.

> @@ +890,4 @@
> >  
> > +      let badge = icon.counter || "";
> > +      if (toolbarButton.getAttribute("badge") != badge)
> > +        toolbarButton.setAttribute("badge", badge);
> 
> Is this badge text accessible anywhere for screen readers? From what I read,
> many screen readers don't see generated content because it doesn't exist in
> the markup.
> 
> Also, I don't see any ARIA/a11y code here to notify users of changes.  It
> seems like ARIA live regions or the XUL equivalent should be used for the
> badges. I think the labels were readable in the previous implementation so
> this may cause a regression. I'm fine with a11y of badges being fixed in a
> follow-up where you can get more input from others.

I started to add this after receiving your feedback, but I saw that I may need to add a user-facing string which would block uplifting. I think we could set aria-live="polite" and use aria-label="Messages (2)" for example, where the badge text succeeds the toolbar button label.

I filed bug 824785 for this.

> @@ +943,5 @@
> >  
> >      panel.addEventListener("popupshown", function onpopupshown() {
> >        panel.removeEventListener("popupshown", onpopupshown);
> >        aToolbarButtonBox.setAttribute("open", "true");
> > +      aToolbarButtonBox.parentNode.setAttribute("open", "true");
> 
> Is this attribute actually needed on both elements at this time?

Yeah it's needed. I added a comment to the code to explain this.

> ::: browser/base/content/test/social/browser_social_mozSocial_API.js
> @@ +27,5 @@
> >          triggerIconPanel();
> >      }
> >  
> >      function triggerIconPanel() {
> > +      let statusIcon = document.querySelector("#social-toolbar-item > .social-notification-container:nth-child(2) > .toolbarbutton-1");

Left over from the previous way the markup was written. I removed it from this patch.

> ::: browser/themes/pinstripe/browser.css
> @@ +3790,5 @@
> >  }
> >  
> >  /* === social toolbar button === */
> >  
> > +toolbar[mode="icons"] .social-notification-container {
> 
> How about toolbar[mode="icons"] > #social-toolbar-item >
> .social-notification-container for improved perf.

Done.
 
> @@ +3803,1 @@
> >    margin-left: 0;
> 
> I think you also want this to match against the jewels which are no longer
> children of the #social-toolbar-item.  Right now this patch causes an extra
> 4px of margin on the left and right of the jewels on OS X.

I'm not sure what you mean here. I don't see how the jewels can not be children of the #social-toolbar-item. I also don't see any extra margins due to this patch.
 
> > +.social-notification-container > .toolbarbutton-1[badge]:not([badge=""])::after {
> > +  content: attr(badge);
> 
> The "content" property probably belongs in browser/base/ otherwise no badge
> text will be shown on 3rd party themes unless they know to copy this.

Done.

> @@ +3825,4 @@
> >    font-size: 9px;
> >    font-weight: bold;
> >    padding: 0 1px;
> > +  color: #fff;
> 
> Nit: why the change from "white"?

Personal preference, and it uses one less byte ;)
 
> @@ +3843,5 @@
> > +toolbar[mode="icons"] .social-notification-container > .toolbarbutton-1[badge]:not([badge=""])::after {
> > +  right: -2px;
> > +}
> > +
> > +toolbar[mode="icons"] .social-notification-container > .toolbarbutton-1[badge]:not([badge=""]):-moz-locale-dir(rtl)::after {
> 
> Use child selectors with #social-toolbar-item in these two places too.

Done.

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 695823 [details] [diff] [review]
Patch v1.1

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

untested, but looks like this is quite an improvement.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 695823 [details] [diff] [review]
Patch v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: broken looking social api toolbar buttons if users are in text or text&icons mode
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): only affects social api
String or UUID changes made by this patch: none

Requesting aurora-approval early since it is nearing the end of the Aurora lifecycle.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1 (for Fx20)

This patch is rebased on top of the multi-provider changes. If this gets r+ from Mark or Matt I'll go ahead and land to make sure that it makes Fx20.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for Aurora Fx19

This is the same as Patch v1.1 except it is rebased for Aurora 19. I will request approval once the v1.1 for m-c gets r+.

Comment 23

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 696446 [details] [diff] [review]
Patch v1.1 (for Fx20)

I think the code here is great.  However, the problem described in the bug title doesn't seem actually fixed.  Whenever toolbar item text is shown, the toolbar still looks very "stretched" - not the icons, but the text itself.  Eg, using facebook, the text for the buttons is "Facebook Messenger", "See All Friend Requests", "See All Messages" and "See All Notifications".  The width of the text causes each of the buttons to be ~3x the width of the other buttons in the toolbar.

I understand this text comes from the providers, but given the bug title I can't give r+.  We are currently using the same text for the buttons as for the tooltip, which can't really work.  We probably need to change our API so providers can supply a "single word" title of the button.  If such a bug was opened, and this patch was considered "part 1" of the fix with "part 2" actually using the new provider-supplied string, I'd go r+.

Comment 24

[Mark Hammond [:markh] [:mhammond]]

Given comment 23, this needs an API change.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

I think that's unnecessary scope creep - these modes are rarely used (and we're getting rid of them in bug 573329), so we don't need to have this bug cover an API change to better support them. I think the bar here is "not horrendously broken", and it seems like this patch mostly gets us there. If there are simple things we can do to further improve this (truncate labels? omit labels in icons+text mode?), by all means lets do that, but requiring API changes solely to support this seems unreasonable.

Comment 26

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #25)
> I think that's unnecessary scope creep

Fair enough.  I'd be happy with omitting the text for all buttons other than the provider itself, which I think would still look reasonable.  Alternatively, I'll r+ if the consensus is the patch meets the "not horrendously broken" bar as it stands (which I guess it does - IMO it's "badly" broken, but probably not "horrendously" :)

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Hiding the labels is insufficient for Text mode, since the buttons are presently blank in Text mode.

I think it would look wrong to hide the text labels in Text & Icons mode for these buttons. I would rather show longer labels (like this patch ends up doing) than not showing any labels when the user requests them.

Comment 28

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 696446 [details] [diff] [review]
Patch v1.1 (for Fx20)

Given I'm happy with the code and that Jaws and Gavin are responsible for, and happy with the "policy", I'm changing my review to r+.

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=7d8b946dc532

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05b4fc0bfe2c

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/05b4fc0bfe2c

Comment 31

[Matthew N. [:MattN]]

Attachment: Screenshot of extra-wide jewels mentioned in comment 16

(In reply to Jared Wein [:jaws] from comment #17)
> > @@ +3803,1 @@
> > >    margin-left: 0;
> > 
> > I think you also want this to match against the jewels which are no longer
> > children of the #social-toolbar-item.  Right now this patch causes an extra
> > 4px of margin on the left and right of the jewels on OS X.
> 
> I'm not sure what you mean here. I don't see how the jewels can not be
> children of the #social-toolbar-item. I also don't see any extra margins due
> to this patch.

See the attached screenshot. This should probably be fixed before further uplift unless the increased width is desired.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

I filed bug 828120 (I see that too).

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for Aurora Fx20

[Approval Request Comment]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: broken looking social api toolbar buttons if users are in text or text&icons mode
Testing completed (on m-c, etc.): landed on m-c at beginning of 21 cycle
Risk to taking this patch (and alternatives if risky): only affects social api
String or UUID changes made by this patch: none

This patch also includes the patch for bug 828120 since it was a trivial followup and will be necessary to uplift as well. This patch is pretty invasive, and thus I think we should not uplift to BetaFx19.

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

I'd like to stop tracking this bug for Firefox 19 but start tracking it for Firefox 20 based on the complexity/risk of the patch and the fact that this bug was not introduced with Firefox 19.

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

Try push for the Aurora patch, https://tbpl.mozilla.org/?tree=Try&rev=e91c5729effa

Comment 36

[bhavana bajaj [:bajaj]]

not tracking for Fx19 , considering comment# 34.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5a2eff69fb35

Comment 38

[Mihai Morar, (:MihaiMorar)]

I confirm the fix is verified on FF 20.b1 on Windows 7 x64, Ubuntu 12.04 x86 and Mac OS 10.8:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0

Comment 39

[Mihai Morar, (:MihaiMorar)]

Verified Fixed on Latest Aurora 21.0a2 on Windows 7 x64 and Mac OS 10.8