1004930 - Generic way to add buttons for actions to a conversation

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, enhancement)

Description

[Benedikt Pfeifer]

By flo [1]:
Some actions associated with conversations need discoverable (ie. visible on screen) ways to start them. For example file transfers and starting a video call are almost in the same situation for the UI to start them.

Given the way our conversations are currently displayed (with different layouts based on the size of the window), displaying an icon is non-trivial, so I was thinking that work shouldn't be duplicated.

The icons might end up near the "target switcher" icon that's on conversations already.

[1] http://log.bezut.info/instantbird/140502/#m77

Comment 1

[Florian Quèze [:florian]]

(In reply to Benedikt Pfeifer [:Mic] from comment #0)

> The icons might end up near the "target switcher" icon that's on
> conversations already.

The "target switcher" (that some people call "protocol icon") is also a button, that should be displayed using the generic way that will be added in this bug.

Comment 2

[aleth [:aleth]]

I would suggest styling these buttons a bit like those in the FX URL bar - i.e. they are icons which show they are buttons on hover.

Comment 3

[Mayank Kumar [:mayanktg]]

Attachment: WIP: Generic buttons patch

Comment 4

[aleth [:aleth]]

Have you tried making the conversation window very small and then bringing it back to its normal size, to check whether your buttons survive the binding changes?

Comment 5

[Mayank Kumar [:mayanktg]]

Attachment: WIP: Generic buttons patch

Changesets:
* added class "convToolbarbutton" which styles toolbarbutton similar to FF Toolbar buttons.
* disabled attribute hides the button from the bar.

Error present:
* In target switcher menupopup, the menuitems getting repeated twice.

Comment 6

[Mayank Kumar [:mayanktg]]

Attachment: Screenshot for toolbarbuttons

Comment 7

[Mayank Kumar [:mayanktg]]

Attachment: Screenshot - wide

Comment 8

[Mayank Kumar [:mayanktg]]

Attachment: WIP: Generic buttons patch

Comment 9

[Patrick Cloke [:clokep]]

Comment on attachment 8430939 [details] [diff] [review]
WIP: Generic buttons patch

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

::: im/content/conversation.xml
@@ +2015,4 @@
>          <xul:description anonid="displayName" class="displayName" flex="1"
>                           crop="end" xbl:inherits="value=displayName"/>
> +        <xul:hbox id="convToolbarbuttonBox">
> +          <children/>

I had a brief conversation on IRC about this, but why didn't we just put:
<children/>
<xul:toolbarbutton class="prplIcon alltargets-button"
...
</xul:toolbarbutton>

Instead of dynamically adding it in JS?

Comment 10

[Mayank Kumar [:mayanktg]]

Attachment: Generic buttons patch

Comment 11

[Mayank Kumar [:mayanktg]]

(In reply to Patrick Cloke [:clokep] from comment #9)
> Comment on attachment 8430939 [details] [diff] [review]
> WIP: Generic buttons patch
> 
> Review of attachment 8430939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: im/content/conversation.xml
> @@ +2015,4 @@
> >          <xul:description anonid="displayName" class="displayName" flex="1"
> >                           crop="end" xbl:inherits="value=displayName"/>
> > +        <xul:hbox id="convToolbarbuttonBox">
> > +          <children/>
> 
> I had a brief conversation on IRC about this, but why didn't we just put:
> <children/>
> <xul:toolbarbutton class="prplIcon alltargets-button"
> ...
> </xul:toolbarbutton>
> 
> Instead of dynamically adding it in JS?
We added it using JS because we thought that duplicating the code for buttons at two place shouldn't occur. Sorry I confused you with "repeating JS code". We'd do what's suitable here and go with your and Mic's decision :)

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8431334 [details] [diff] [review]
Generic buttons patch

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

::: im/content/conversation.xml
@@ +1740,5 @@
> +          // Target switcher button.
> +          let targetSwitcher = document.createElement("toolbarbutton");
> +          targetSwitcher.classList.add("prplIcon");
> +          targetSwitcher.classList.add("alltargets-button");
> +          targetSwitcher.setAttribute("context", "");

What's the point of setting context to ""? (could use a comment)

@@ -2011,5 @@
>          <![CDATA[
>            this.topic
>                .addEventListener("click", this.startEditTopic.bind(this));
> -          if (this.hasAttribute("allowTargetChange"))
> -            this.allowTargetChange();

What makes this no longer necessary?
(note: I'm not convinced this allowTargetChange stuff works correctly; neither before nor after the patch)

Comment 13

[Mayank Kumar [:mayanktg]]

Thanks for taking a look into this patch. :)
> ::: im/content/conversation.xml
> @@ +1740,5 @@
> > +          // Target switcher button.
> > +          let targetSwitcher = document.createElement("toolbarbutton");
> > +          targetSwitcher.classList.add("prplIcon");
> > +          targetSwitcher.classList.add("alltargets-button");
> > +          targetSwitcher.setAttribute("context", "");
> 
> What's the point of setting context to ""? (could use a comment)
Mic said not to remove any of the attribute which was previously defined in the target switcher toolbarbutton. So I kept the context="" attribute here. Maybe we don't need it here. 
> @@ -2011,5 @@
> >          <![CDATA[
> >            this.topic
> >                .addEventListener("click", this.startEditTopic.bind(this));
> > -          if (this.hasAttribute("allowTargetChange"))
> > -            this.allowTargetChange();
> 
> What makes this no longer necessary?
> (note: I'm not convinced this allowTargetChange stuff works correctly;
> neither before nor after the patch)
I removed this line from the constructor as it was causing the target switcher menu to contain repeated contact. Removing the allowTargetChange() fixed this issue and target switcher was working correctly upto my knowledge. Please see and suggest how can I make allowTargetChange work correctly.

I will be updating the patch in a few hours, also please then comment regarding the CSS of the patch. I'm not sure about it and changes need to be done for the divider line and the button css (I remember you commented about having too much gap between the buttons).

Comment 14

[Florian Quèze [:florian]]

(In reply to Mayank Kumar [:mayanktg] from comment #13)
> Thanks for taking a look into this patch. :)
> > ::: im/content/conversation.xml
> > @@ +1740,5 @@
> > > +          // Target switcher button.
> > > +          let targetSwitcher = document.createElement("toolbarbutton");
> > > +          targetSwitcher.classList.add("prplIcon");
> > > +          targetSwitcher.classList.add("alltargets-button");
> > > +          targetSwitcher.setAttribute("context", "");
> > 
> > What's the point of setting context to ""? (could use a comment)
> Mic said not to remove any of the attribute

And I'm saying the code setting this attribute could use a comment.

> Maybe we don't need it here. 

I think we do.

Comment 15

[Florian Quèze [:florian]]

(In reply to Mayank Kumar [:mayanktg] from comment #13)

> please then comment
> regarding the CSS of the patch. I'm not sure about it and changes need to be
> done for the divider line and the button css (I remember you commented about
> having too much gap between the buttons).

7px of padding on both sides of each button seems excessive.

Comment 16

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v2: Generic buttons patch

I have changed the width of the toolbarbutton with a padding of 4px which looks reasonable now.
The line name divider wasn't working correctly at small width of the window in the previous patch which I've corrected.

Comment 17

[Mayank Kumar [:mayanktg]]

Attachment: (WIP) v3: Generic buttons patch

Comment 18

[Mayank Kumar [:mayanktg]]

Attachment: Screenshot with DOMi check.

Comment 19

[Florian Quèze [:florian]]

Comment on attachment 8445454 [details] [diff] [review]
(WIP) v3: Generic buttons patch

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

::: im/content/conversation.xml
@@ +1733,5 @@
>        <method name="initConversationUI">
>          <body>
>          <![CDATA[
> +          let cti = this.getElt("conv-top-info");
> +          // Include new buttons here. Class "convToolbarbutton" should be added

What does "Include new buttons here." mean?

The class convToolbarbutton is defined in the css file, but never used. Is there a reason why it's not used for the target switcher?

@@ +1746,5 @@
> +                                      account.protocol.iconBaseURI + "icon.png");
> +          let targetTooltip =
> +            this.bundle.formatStringFromName("targetTooltipChat",
> +                                             [this._conv.title, account.name,
> +                                             account.protocol.name], 3);

The indent isn't correct on this line.

@@ +1752,5 @@
> +          let targetPopup = document.createElement("menupopup");
> +          targetPopup.classList.add("all-targets-popup");
> +          targetPopup.setAttribute("postion", "after_end");
> +          targetSwitcher.appendChild(targetPopup);
> +          cti.appendChild(targetSwitcher);

Move the |let cti = ...| line right before this one.

::: im/themes/conversation.css
@@ +117,5 @@
> +  padding: 3px 4px;
> +}
> +
> +.convToolbarbutton:hover:not([disabled="true"]) {
> +  background: hsla(0,0%,100%,0.5) linear-gradient(hsla(0,0%,100%,0.1), hsla(0,0%,100%,0.2)) padding-box;

We usually have a space after each , in CSS code, but if this whole block has been copied as-is, it's probably a good idea to keep it like the original.
If it's an exact copy of code existing somewhere else, we may want to add a comment saying where the code is coming from.

Comment 20

[Mayank Kumar [:mayanktg]]

(In reply to Florian Quèze [:florian] [:flo] from comment #19)
> Comment on attachment 8445454 [details] [diff] [review]
> (WIP) v3: Generic buttons patch
> 
> Review of attachment 8445454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: im/content/conversation.xml
> @@ +1752,5 @@
> > +          let targetPopup = document.createElement("menupopup");
> > +          targetPopup.classList.add("all-targets-popup");
> > +          targetPopup.setAttribute("postion", "after_end");
> > +          targetSwitcher.appendChild(targetPopup);
> > +          cti.appendChild(targetSwitcher);
> 
> Move the |let cti = ...| line right before this one.
Other toolbarbuttons would be added to the "conv-top-info" too.
That's why I have initialized it at the beginning so that we don't
have to change it again afterwards. 
> 
> ::: im/themes/conversation.css
> @@ +117,5 @@
> > +  padding: 3px 4px;
> > +}
> > +
> > +.convToolbarbutton:hover:not([disabled="true"]) {
> > +  background: hsla(0,0%,100%,0.5) linear-gradient(hsla(0,0%,100%,0.1), hsla(0,0%,100%,0.2)) padding-box;
> 
> We usually have a space after each , in CSS code, but if this whole block
> has been copied as-is, it's probably a good idea to keep it like the
> original.
> If it's an exact copy of code existing somewhere else, we may want to add a
> comment saying where the code is coming from.
The width has been changed for this button and though I have copied it from
http://mxr.mozilla.org/comm-central/source/mozilla/browser/themes/windows/browser.css
the lines are at different part of the file. I have mentioned about this in
the comment in the patch.

Comment 21

[Mayank Kumar [:mayanktg]]

Attachment: v4: Generic buttons patch.

Changesets:
* Included convToolbarbutton for targetSwitcher.
* Fixed an earlier regression which caused the targetSwticher button's icon to have extra margin in active state, due to which the button shifted left by 2px.

Comment 22

[Florian Quèze [:florian]]

Comment on attachment 8457477 [details] [diff] [review]
v4: Generic buttons patch.

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

::: im/content/conversation.xml
@@ +2017,4 @@
>          <xul:description anonid="displayName" class="displayName" flex="1"
>                           crop="end" xbl:inherits="value=displayName"/>
> +        <!-- Set "context" else context attribute from parent node is used. -->
> +        <xul:hbox id="convToolbarbuttonBox" mousethrough="always" context="">

You can't use the id attribute on anonymous node.

@@ +2245,5 @@
>        <xul:description anonid="statusMessageWithDash"
>                         class="statusMessageWithDash"
>                         xbl:inherits="value=statusMessageWithDash,tooltiptext=statusTooltiptext,editable=topicEditable,editing,noTopic,context=topicContext,role=topicRole"
>                         crop="end" flex="100000"/>
> +      <xul:hbox id="convToolbarbuttonBox" mousethrough="always" context="">

Same comment here.

::: im/themes/conversation.css
@@ +99,5 @@
> +.alltargets-button > .toolbarbutton-icon {
> +  margin: 0;
> +}
> +
> +.alltargets-button >  .all-targets-popup {

Nit: double space.

@@ +104,1 @@
>    cursor: pointer;

What's the point of this rule? We used to show an hand cursor on the target selector so that it was visible that it was a click target. Now that the button has an hover effect, I don't think we need this anymore.

@@ +108,5 @@
>    font-weight: bold;
>  }
>  
> +/* The Australis themed button is borrowed from the Firefox toolbarbutton.
> +   http://mxr.mozilla.org/comm-central/source/mozilla/browser/themes/windows/browser.css

You don't need a full url, the path within mozilla-central (ie. "browser/themes/windows/browser.css") is enough.

@@ +110,5 @@
>  
> +/* The Australis themed button is borrowed from the Firefox toolbarbutton.
> +   http://mxr.mozilla.org/comm-central/source/mozilla/browser/themes/windows/browser.css
> +   The properties are set in class toolbarbutton-1 of the CSS file. */
> +.convToolbarbutton {

could this be
.convToolbarbuttonBox > toolbarbutton

and then you wouldn't need to add a class to each of the toolbarbutton elements?

@@ +131,5 @@
> +  background-image: linear-gradient(transparent, rgba(0,0,0,0.15)) !important;
> +  border: 1px solid hsla(0,0%,0%,0.5);
> +  box-shadow: 0 1px 0 hsla(0,0%,100%,0.5),
> +              0 1px 0 hsla(0,0%,0%,0.05) inset,
> +              0 1px 1px hsla(0,0%,0%,0.2) inset;

The border-radius: 3px; line is needed here too, otherwise the button's border switches to square corner if you mouse down on the button, and move the mouse away.

@@ +134,5 @@
> +              0 1px 0 hsla(0,0%,0%,0.05) inset,
> +              0 1px 1px hsla(0,0%,0%,0.2) inset;
> +}
> +
> +#convToolbarbuttonBox {

Looks like you wanted this to be a class, rather than an id.

@@ +135,5 @@
> +              0 1px 1px hsla(0,0%,0%,0.2) inset;
> +}
> +
> +#convToolbarbuttonBox {
> +  padding: 1px 0 2px 0;

add here:
 margin-right: 2px;

@@ +136,5 @@
> +}
> +
> +#convToolbarbuttonBox {
> +  padding: 1px 0 2px 0;
> +  border-bottom: 1px solid rgba(0,0,0,0.25);

This is not correct. Resize your window to a height smaller than 250px and see that border become visible.
Could this border be moved to the parent hbox that contains both the username and the set of icons?

Comment 23

[Mayank Kumar [:mayanktg]]

Attachment: v5: Generic buttons patch.

Changests:
* removed separate border-bottom for displayName and convToolbarbuttonBox.
* removed class convToolbarbutton
* fixed the CSS when the window is less than 250px.

Comment 24

[Mayank Kumar [:mayanktg]]

Attachment: screenshots of toolbarbuttons at different window size

Sorry for the ugly formatting.

Comment 25

[Mayank Kumar [:mayanktg]]

(In reply to Florian Quèze [:florian] [:flo] from comment #22)
> Comment on attachment 8457477 [details] [diff] [review]
> v4: Generic buttons patch.
> 
> Review of attachment 8457477 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: im/themes/conversation.css
> could this be
> .convToolbarbuttonBox > toolbarbutton
> 
> and then you wouldn't need to add a class to each of the toolbarbutton
> elements?
> 
Removing the class idea really helped. It worked with |.conv-top-info > toolbarbutton| instead since the toolbarbuttons are child of conv-top-info and not convtoolbarbuttonBox. :)
> @@ +136,5 @@
> > +}
> > +
> > +#convToolbarbuttonBox {
> > +  padding: 1px 0 2px 0;
> > +  border-bottom: 1px solid rgba(0,0,0,0.25);
> 
> This is not correct. Resize your window to a height smaller than 250px and
> see that border become visible.
> Could this border be moved to the parent hbox that contains both the
> username and the set of icons?
Did as suggested. The borders are unified and the buttons look much better at smaller window size now.

Comment 26

[Florian Quèze [:florian]]

Comment on attachment 8458138 [details] [diff] [review]
v5: Generic buttons patch.

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

The margins/paddings need more work. The gray line and display name shouldn't move compared to where they are without the patch. Also, the height of the toolbar of small windows shouldn't increase (the point of having that small window mode is to have as much space as possible for the conversation content; we don't want to waste the space there with margins).

When the menupopup of the target switcher is visible, the button should stay in the active state, even if the mouse is no longer above the menu.

::: im/content/conversation.xml
@@ +2009,5 @@
>                   xbl:inherits="status,tooltiptext=statusTypeTooltiptext"/>
>      </xul:stack>
>      <xul:stack class="displayNameAndstatusMessageStack"
>                 mousethrough="always" flex="1">
> +      <xul:hbox align="right" flex="1" class="displayNameAndconvToolbarbuttonBox">

This class name doesn't have a correct case. It would be AndConv, not Andconv.
This name also seems too long. What about just displayNameAndToolbar?

@@ +2014,4 @@
>          <xul:description anonid="displayName" class="displayName" flex="1"
>                           crop="end" xbl:inherits="value=displayName"/>
> +        <!-- Set "context" else context attribute from parent node is used. -->
> +        <xul:hbox class="convToolbarbuttonBox" mousethrough="always" context="">

Maybe just "convToolbar" here?

::: im/themes/conversation.css
@@ +96,5 @@
>    display: none;
>  }
>  
> +.alltargets-button > .toolbarbutton-icon {
> +  margin: 0;

What is this for?

@@ +133,5 @@
> +}
> +
> +.displayNameAndconvToolbarbuttonBox {
> +  border-bottom: 1px solid rgba(0,0,0,0.25);
> +  margin: 1px 0 19px 0 !important;

This needs a right margin.

Comment 27

[Mayank Kumar [:mayanktg]]

Attachment: v6: Generic buttons patch.

Comment 28

[Mayank Kumar [:mayanktg]]

(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> Comment on attachment 8458138 [details] [diff] [review]
> v5: Generic buttons patch.
> 
> Review of attachment 8458138 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: im/themes/conversation.css
> @@ +96,5 @@
> >    display: none;
> >  }
> >  
> > +.alltargets-button > .toolbarbutton-icon {
> > +  margin: 0;
> 
> What is this for?
When toolbarbutton is of type menu a menu-dropmarker is added. Even on setting display:none, its leaving a margin which causes target switcher button to have uneven right/left padding. That's why I had to reset the icon with margin: 0.

Comment 29

[Florian Quèze [:florian]]

(In reply to Mayank Kumar [:mayanktg] from comment #28)
> (In reply to Florian Quèze [:florian] [:flo] from comment #26)

> > > +.alltargets-button > .toolbarbutton-icon {
> > > +  margin: 0;
> > 
> > What is this for?
> When toolbarbutton is of type menu a menu-dropmarker is added. Even on
> setting display:none, its leaving a margin which causes target switcher
> button to have uneven right/left padding. That's why I had to reset the icon
> with margin: 0.

Ok, this is non-obvious and I guess it took you some time to figure it out. It sounds like it's worth keeping that knowledge with a comment in the code.

Comment 30

[Mayank Kumar [:mayanktg]]

Attachment: v7: Generic buttons patch.

Comment 31

[Florian Quèze [:florian]]

Attachment: CSS tweaks

I tested this on Mac and tweaked the CSS until things looked good.

Comment 32

[Florian Quèze [:florian]]

Attachment: Patch v8

Attachment 8498505 [details] [diff] folded into attachment 8463561 [details] [diff] [review].

Comment 33

[Florian Quèze [:florian]]

Comment on attachment 8498510 [details] [diff] [review]
Patch v8

Would still be nice to have someone testing this on Windows.

Comment 34

[aleth [:aleth]]

Comment on attachment 8498510 [details] [diff] [review]
Patch v8

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

I haven't tried this patch, but the code looks fine to me. r+ with added comments to avoid future confusion.

::: im/themes/conversation.css
@@ +169,5 @@
> +    margin: 2px 0 2px 2px;
> +  }
> +
> +  .displayNameAndstatusMessageStack {
> +    -moz-margin-start: 4px;

A comment here might be helpful to explain what this is for.

@@ +190,5 @@
>    }
>  
>    .displayName {
>      font-size: large;
> +    margin: 5px 0 0 0 !important;

and here (magic value of 5)

@@ +199,5 @@
>      margin: 32px 0 0;
>    }
>  
> +  .convToolbar {
> +    margin: -1px 0 0;

What are we compensating?

Comment 35

[Patrick Cloke [:clokep]]

http://hg.mozilla.org/comm-central/rev/57a529a4633e

Comment 36

[Patrick Cloke [:clokep]]

Fix typo; https://hg.mozilla.org/comm-central/rev/6300e54fef35