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

VERIFIED FIXED in Firefox 20

Status

()

Firefox
SocialAPI
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Ibrahima SARR, Assigned: jaws)

Tracking

17 Branch
Firefox 21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18+ wontfix, firefox19- wontfix, firefox20+ verified, firefox21 verified)

Details

(Whiteboard: [1.5][testday-20130301])

Attachments

(4 attachments, 9 obsolete attachments)

32.87 KB, image/png
Details
25.07 KB, patch
markh
: review+
Details | Diff | Splinter Review
8.05 KB, image/png
Details
25.08 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 681616 [details]
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

Updated

5 years ago
Component: Untriaged → SocialAPI
Are you using any add-ons/themes that might cause this? Does it reproduce in Safe mode or with a clean profile?
(Reporter)

Comment 2

5 years ago
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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: SocialApi icons on Toolbar not displaying correclty → Customizing toolbar to show "Text" or "Icons & Text" breaks the Social toolbar UI
(Reporter)

Comment 4

5 years ago
Exactly! Thanks for updating the title of the bug.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Whiteboard: [1.5]
tracking-firefox18: --- → +
tracking-firefox19: --- → +
Created attachment 690968 [details] [diff] [review]
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.
Created attachment 691950 [details] [diff] [review]
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.
Attachment #690968 - Attachment is obsolete: true
Created attachment 692016 [details] [diff] [review]
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.
Attachment #691950 - Attachment is obsolete: true
Created attachment 692561 [details] [diff] [review]
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).
Attachment #692016 - Attachment is obsolete: true
Created attachment 693593 [details] [diff] [review]
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.
Attachment #692561 - Attachment is obsolete: true
Attachment #693593 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #693593 - Flags: feedback?(mconley)
Created attachment 693601 [details] [diff] [review]
Patch v.3.1

With proper test adjustments.
Attachment #693593 - Attachment is obsolete: true
Attachment #693593 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #693593 - Flags: feedback?(mconley)
Attachment #693601 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #693601 - Flags: feedback?(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.
Attachment #693601 - Flags: feedback?(mconley)
Created attachment 693983 [details] [diff] [review]
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.
Attachment #693601 - Attachment is obsolete: true
Attachment #693601 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #693983 - Flags: review?(mnoorenberghe+bmo)
Attachment #693983 - Flags: review?(mhammond)
(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.
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.
status-firefox18: --- → wontfix
(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 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.
Attachment #693983 - Flags: review?(mnoorenberghe+bmo) → review-
Created attachment 695823 [details] [diff] [review]
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.
Attachment #693983 - Attachment is obsolete: true
Attachment #693983 - Flags: review?(mhammond)
Attachment #695823 - Flags: review?(mnoorenberghe+bmo)
Attachment #695823 - Flags: review?(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.
Attachment #695823 - Flags: review?(mhammond) → review+
Duplicate of this bug: 824673
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.
Attachment #695823 - Flags: approval-mozilla-aurora?
Created attachment 696446 [details] [diff] [review]
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.
Attachment #695823 - Attachment is obsolete: true
Attachment #695823 - Flags: review?(mnoorenberghe+bmo)
Attachment #695823 - Flags: approval-mozilla-aurora?
Attachment #696446 - Flags: review?(mnoorenberghe+bmo)
Attachment #696446 - Flags: review?(mhammond)
Created attachment 696459 [details] [diff] [review]
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 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+.
Attachment #696446 - Flags: review?(mhammond) → review-
Given comment 23, this needs an API change.
Whiteboard: [1.5] → [1.5][needs-api-change]
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.
Whiteboard: [1.5][needs-api-change] → [1.5]
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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" :)
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 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+.
Attachment #696446 - Flags: review- → review+
Try push: https://tbpl.mozilla.org/?tree=Try&rev=7d8b946dc532

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05b4fc0bfe2c
Attachment #696446 - Flags: review?(mnoorenberghe+bmo)
https://hg.mozilla.org/mozilla-central/rev/05b4fc0bfe2c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Created attachment 699453 [details]
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 828120
I filed bug 828120 (I see that too).
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Created attachment 701246 [details] [diff] [review]
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.
Attachment #696459 - Attachment is obsolete: true
Attachment #701246 - Flags: approval-mozilla-aurora?
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.
status-firefox19: --- → affected
status-firefox20: --- → affected
tracking-firefox19: + → ?
tracking-firefox20: --- → ?
Try push for the Aurora patch, https://tbpl.mozilla.org/?tree=Try&rev=e91c5729effa
not tracking for Fx19 , considering comment# 34.
status-firefox19: affected → wontfix
tracking-firefox19: ? → -
tracking-firefox20: ? → +

Updated

5 years ago
Attachment #701246 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/5a2eff69fb35
status-firefox20: affected → fixed
status-firefox21: --- → fixed
Blocks: 829868
Blocks: 829416
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
status-firefox20: fixed → verified
Whiteboard: [1.5] → [1.5][testday-20130301]
Verified Fixed on Latest Aurora 21.0a2 on Windows 7 x64 and Mac OS 10.8
Status: RESOLVED → VERIFIED
status-firefox21: fixed → verified
You need to log in before you can comment on or make changes to this bug.