Last Comment Bug 790112 - toolbar button doesn't reflect state of distinct buttons inside it
: toolbar button doesn't reflect state of distinct buttons inside it
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 18
Assigned To: (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
:
Mentors:
Depends on: 797667 799749
Blocks: 788598
  Show dependency treegraph
 
Reported: 2012-09-10 16:53 PDT by Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
Modified: 2013-12-27 14:29 PST (History)
7 users (show)
gavin.sharp: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
First attempt at getting the desired styling on Windows (4.12 KB, patch)
2012-09-13 00:58 PDT, Mark Hammond [:markh]
no flags Details | Diff | Review
kinda working with regular toolbar items. (12.40 KB, text/plain)
2012-09-14 00:35 PDT, Mark Hammond [:markh]
no flags Details
updated (10.32 KB, patch)
2012-09-16 23:52 PDT, Mark Hammond [:markh]
jaws: feedback+
Details | Diff | Review
Patch (18.44 KB, patch)
2012-09-25 21:34 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review
working on osx (23.12 KB, patch)
2012-09-26 13:50 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
Patch v2 (22.76 KB, patch)
2012-09-26 16:50 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
felipc: review+
Details | Diff | Review
Patch V3 (25.36 KB, patch)
2012-09-27 17:03 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
felipc: review+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Review

Description Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-09-10 16:53:06 PDT
hover/click events affect the button group, not the individual buttons within it.  We need to make each individual button support the events.  At a simple level, this should be achievable with changes in css alone.  If the design further changes the button layout, that may also require some xul and possibly code changes.
Comment 1 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-11 02:15:08 PDT
I don't think we should change this to have individual buttons.

Switching to distinct buttons introduces different problems that don't have obvious solutions.

With distinct buttons:
1) Can a user customize their toolbar and remove one of the buttons?
2) Can a user place a different button between the various buttons?
3) How does a user know that a specific button is related to a provider?

This isn't an obvious win by switching to a distinct button implementation. I'd be fine with a large button that has the inner-buttons separated by a vertical splitter, similar to the proposed zoom controls button in Australis. I think that would be a good compromise.
Comment 2 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-09-11 06:57:08 PDT
(In reply to Jared Wein [:jaws] from comment #1)
> I don't think we should change this to have individual buttons.

This is not about creating individual toolbarbutton elements for each status icon.  Simple css tweaks can achieve what we want on the existing button.
Comment 3 Mark Hammond [:markh] 2012-09-13 00:58:19 PDT
Created attachment 660739 [details] [diff] [review]
First attempt at getting the desired styling on Windows

My CSS foo is weak - be gentle ;)  There may be a better way of achieving this (but I want my CSS foo to be stronger, so I had a go anyway :)

The changes are mainly to CSS.  I introduced a new class "social-toolbar-icon-container" which is used both by the provider icon and the dynamic ambient icons.  A couple of code changes were necessary - using this new class and dynamically adding the "open" attribute to only the individual icon instead of the entire toolbar button.

This only touches winstripe - I thought I'd get feedback on the general strategy before doing the same to the other themes.  But it seems to work well there and has the desired effect (although the vertical line between the icons may want a higher contrast)
Comment 4 Dão Gottwald [:dao] 2012-09-13 01:46:54 PDT
The obvious solution is to put individual <toolbarbutton>s in one <toolbaritem>, and drop your custom, hard-to-maintain CSS.

(In reply to Jared Wein [:jaws] from comment #1)
> With distinct buttons:
> 1) Can a user customize their toolbar and remove one of the buttons?

No.

> 2) Can a user place a different button between the various buttons?

No.

> 3) How does a user know that a specific button is related to a provider?

I don't understand this point.
Comment 5 Mark Hammond [:markh] 2012-09-13 04:01:59 PDT
(In reply to Dão Gottwald [:dao] from comment #4)
> The obvious solution is to put individual <toolbarbutton>s in one
> <toolbaritem>, and drop your custom, hard-to-maintain CSS.

Yeah - makes perfect sense.  Now you mention it, notice that the left-most "provider icon" uses a <menupopup> (and thus gets no arrow on the popup) whereas the dynamic area is an anchored panel (and thus gets the arrow).  We should probably be consistent there too - but which one?
Comment 6 Mark Hammond [:markh] 2012-09-14 00:35:52 PDT
Created attachment 661149 [details]
kinda working with regular toolbar items.

This is another WIP - CSS is starting to win the battle :)

The patch removes alot of CSS and also removes a fair bit of the code populating the ambient area.  Of note:

* Each of the toolbar icons is now a regular toolbarbutton.  The counter is implemented as the label attribute, with CSS hiding it when the label value is "".  The image is just the regular image attribute.

* The CSS will need some tweaks and might need some love from someone better at CSS than I am - particularly the positioning of both the toolbarbutton label and the separators (borders) between each button (eg, the border is now a few pixels inside the hover region)

* The "provider popup" is now a panel so they all consistently get an anchored arrow on popup - but the "Show social sidebar" item is causing me grief - I can't work out how to make it look like a regular checked menuitem.  If necessary I can revert this part so the provider popup is just a regular toolbar popup.

It would be great if feedback can be offered on whether this approach is likely to fly, or if the previous patch should be continues in the short term and this level of refactoring be done later.  For this reason I haven't obsoleted the previous patch...
Comment 7 Mark Hammond [:markh] 2012-09-16 23:52:54 PDT
Created attachment 661704 [details] [diff] [review]
updated

Attaching a new patch that doesn't attempt to change the provider menu from a <menupopup> to a <panel> - I opened bug 791604 for that.

This looks pretty good to me - one obvious issue is that the "provider" icon has some extra padding on the right which I can't work out how to remove.

This patch still replaces most <button> elements with <toolbarbutton> elements, which simplifies the CSS significantly - about 40 lines of CSS has been removed.  It also simplifies the code so there is a nett reduction in the JS code.  Still only for winstripe but I expect that if this approach is used, porting the CSS changes to the other themes should be fairly simple.

Note the JS code still has some test code in place to show how the counters are shown - see the 2 references to "XXXcounter".
Comment 8 Dão Gottwald [:dao] 2012-09-17 01:51:32 PDT
Comment on attachment 661704 [details] [diff] [review]
updated

>@@ -3358,11 +3310,15 @@
>   color: white;
>   font-size: 9px;
>   font-weight: bold;
>-  position: absolute;
>-  right: -3px;
>-  top: -4px;
>+  position: relative;
>+  margin-left: -15px !important;

Where does this magic number come from?
Comment 9 Mark Hammond [:markh] 2012-09-17 03:47:50 PDT
(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 661704 [details] [diff] [review]
... 
> Where does this magic number come from?

By eye :)  I forgot to mention I was struggling a lot with the "counter" - it certainly needs to be improved.  We just want the label attribute to be drawn in the specified font, overlapping the icon, right-justified aligned to the top-right corner.  It sounds much easier than I found it to implement :(
Comment 10 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-17 10:36:34 PDT
Comment on attachment 661704 [details] [diff] [review]
updated

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

The toolbar buttons don't match the same appearance as other navigation toolbar buttons on winstripe (http://screencast.com/t/bl2vRkV1). Can this be fixed?

::: browser/themes/winstripe/browser.css
@@ +3288,5 @@
> +#social-toolbar-item > #social-toolbar-button-box > #social-status-iconbox > toolbarbutton,
> +#social-provider-button
> +{
> +  padding-left: 0px;
> +  padding-right: 0px;

Removing the padding puts the icons too close to the edge of the button. Can you leave the padding-left on the first toolbarbutton and the padding-right on the last toolbarbutton?

@@ +3310,5 @@
>    color: white;
>    font-size: 9px;
>    font-weight: bold;
> +  position: relative;
> +  margin-left: -15px !important;

I tried playing with position:absolute and top:0, right:0, but that wouldn't work as I expected. I'll let you know if I can think of something that will get this to work as desired.
Comment 11 Mark Hammond [:markh] 2012-09-17 17:09:11 PDT
(In reply to Jared Wein [:jaws] from comment #10)
> Review of attachment 661704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The toolbar buttons don't match the same appearance as other navigation
> toolbar buttons on winstripe (http://screencast.com/t/bl2vRkV1). Can this be
> fixed?

Interesting - I don't see this.  Can you give a little more context (ie, is the strange-looking icon expected but just the sizing wrong?  Is this with your provider?)
Comment 12 Sheila Mooney 2012-09-25 19:07:55 PDT
What's going on with this?
Comment 13 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-25 19:20:06 PDT
I'll finish this up using Mark's patch as a basis. I've got most of it working locally now, just have to get the notification counts placed in the corner.
Comment 14 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-25 21:34:08 PDT
Created attachment 664786 [details] [diff] [review]
Patch

This patch implements the styles for Windows and inserts a separator between each notification button.

I implemented the "hover one button, show hover for all buttons" feature, but it looked awkward since there wasn't separate hover colors for the two hover states. I didn't include this feature in this patch because of this.
Comment 15 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-09-26 10:36:15 PDT
Comment on attachment 664786 [details] [diff] [review]
Patch

I dont think the event listeners for the provider menubutton are necessary any longer, but haven't tested that.

the button getter is less useful now (esp. if the above is true)

on OSX, if the toolbarbuttons are not in the same parent, they will appear as separate buttons.  The patch I worked up removed the hbox in the toolbaritem.  It also means the child removal needs to change.

I didn't use a stack in my approach, but I think I like it better since it avoids playing with positioning.  I just used a xul:box with the toolbarbutton-1 style and got the correct appearance.

On osx there is also a flicker now when clicking on the button.  This is because toolbarbutton-1 depresses the button during the click, releasing it, then onpopupshown depresses it again.  That needs to be addressed.

I'll take the patch and work up the changes for osx.
Comment 16 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-26 10:59:47 PDT
I noticed that we could the iconbox as well, but I think we should handle that in a follow-up.
Comment 17 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-09-26 11:11:44 PDT
(In reply to Jared Wein [:jaws] from comment #16)
> I noticed that we could the iconbox as well, but I think we should handle
> that in a follow-up.

Actually, the problem is a bit bigger.  Since a stack is wrapping the toolbarbutton, the buttons will not appear in a button group.  The buttons need to be siblings for that to work.
Comment 18 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-09-26 13:50:16 PDT
Created attachment 665110 [details] [diff] [review]
working on osx

this patch is based on the last one posted by Jared.  It uses an hbox wrapper with the toolbarbutton-1 class to create the proper platform appearance for osx.  lin/win css not updated.
Comment 19 :Felipe Gomes (needinfo me!) 2012-09-26 14:41:33 PDT
Comment on attachment 664786 [details] [diff] [review]
Patch

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

Writing down the feedback from our pair review:

- rename the iconContainer param to aToolbarButton
- check if popupshown for the provider button is still needed
- missing padding when no ambient icons are shown
- counter box is too high up

::: browser/base/content/browser-social.js
@@ +591,5 @@
> +      let labelValue = icon.counter || "";
> +      // Note we must ensure the button has an attribute 'label' for the
> +      // CSS which hides it to work correctly, hence the hasAttribute check...
> +      if (!label.hasAttribute("value") || label.getAttribute("value") != labelValue)
> +        label.setAttribute("value", labelValue);

this has the same effect of unconditionally setting the attribute
Comment 20 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-26 16:50:55 PDT
Created attachment 665186 [details] [diff] [review]
Patch v2

Thanks for providing the OS X implementation Shane. I had to rework the CSS for Windows (as expected) and also make a couple tweaks to the browser-social portion of the patch.

I tested this patch out on OS X and it looks fine. I think this is ready to land now.
Comment 21 :Felipe Gomes (needinfo me!) 2012-09-26 18:17:35 PDT
Comment on attachment 665186 [details] [diff] [review]
Patch v2

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

This already applies to the existing content, but social-toolbar-

::: browser/base/content/browser-social.js
@@ +539,2 @@
>      if (!SocialUI.haveLoggedInUser()) {
> +      ["social-notification-box"].forEach(function removeChildren(parentId) {

forEach no longer necessary

@@ +544,5 @@
>        });
>      }
> +    while (tbi.lastChild != tbi.firstChild) {
> +      tbi.removeChild(tbi.lastChild);
> +    }

shouldn't this block be inside the if condition above?

::: browser/base/content/browser.xul
@@ +659,5 @@
>                       ondragexit="homeButtonObserver.onDragExit(event)"
>                       onclick="BrowserGoHome(event);"
>                       aboutHomeOverrideTooltip="&abouthome.pageTitle;"/>
>  
> +      <toolbaritem id="social-toolbar-item"

this already applies to the existing content, but I wonder if #social-toolbar-item needs to be added to @primaryToolbarButtons@ as described here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#929

But I believe the answer might be no as it's not a default button
Comment 22 :Felipe Gomes (needinfo me!) 2012-09-26 18:20:34 PDT
r+ with the above things considered and figuring out the linux story (follow-up patch?)
Comment 23 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-09-27 17:03:15 PDT
Created attachment 665709 [details] [diff] [review]
Patch V3

This patch addresses issues from review on V2, and fixes linux css.
Comment 24 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-09-27 17:05:07 PDT
Comment on attachment 665709 [details] [diff] [review]
Patch V3

I know you r+'d, but could you just take a glance through the last changes for sanity purposes.

- linux css
- notification position on osx fixed
- addressed v2 review issues
Comment 25 :Felipe Gomes (needinfo me!) 2012-09-27 23:52:16 PDT
Comment on attachment 665709 [details] [diff] [review]
Patch V3

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

Looks good!
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-28 11:27:11 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/abf5f7d05488
Comment 27 :Ehsan Akhgari (out sick) 2012-09-28 12:43:43 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/41c5a807276f for browser-chrome failures.  Example: https://tbpl.mozilla.org/php/getParsedLog.php?id=15638256&tree=Mozilla-Inbound
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-28 12:45:29 PDT
Hrm, I guess no one ran tests? :(

Looks like it's just a case of http://hg.mozilla.org/mozilla-central/annotate/ed5f37774104/browser/base/content/test/browser_social_mozSocial_API.js#l31 not having been updated.
Comment 29 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-29 01:43:14 PDT
Relanded with test fixes for browser_social_mozSocial_API.js and browser_social_toolbar.js.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c3d164cffc
Comment 30 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-29 03:18:18 PDT
Backed out again due to some test timeouts on inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ccdb1019296
Comment 31 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-02 00:48:06 PDT
Fixed the timeout in the test and pushed to try server to confirm,
https://tbpl.mozilla.org/?tree=Try&rev=4883a3f324a1

https://hg.mozilla.org/integration/mozilla-inbound/rev/13140a27ff09
Comment 32 Ed Morley [:emorley] 2012-10-02 08:57:45 PDT
https://hg.mozilla.org/mozilla-central/rev/13140a27ff09
Comment 33 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-02 18:50:29 PDT
Pushed to mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/8971aa08c2fe

Aurora approved by Gavin offline and also on IRC, 
6:16 PM <gavin> jaws: any chance you could merge the trunk toolbarbutton patch to aurora?

Note You need to log in before you can comment on or make changes to this bug.