Generic way to add buttons for actions to a conversation

RESOLVED FIXED in 1.6

Status

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Mic, Assigned: mayanktg)

Tracking

(Blocks 2 bugs)

Dependency tree / graph

Details

Attachments

(5 attachments, 11 obsolete attachments)

10.12 KB, image/png
Details
16.13 KB, image/png
Details
17.32 KB, image/png
Details
23.57 KB, image/png
Details
12.02 KB, patch
aleth
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
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
(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

5 years ago
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.
Assignee

Comment 3

5 years ago
Posted patch WIP: Generic buttons patch (obsolete) — Splinter Review

Comment 4

5 years ago
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?
Assignee

Comment 5

5 years ago
Posted patch WIP: Generic buttons patch (obsolete) — Splinter Review
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.
Attachment #8429953 - Attachment is obsolete: true
Assignee

Comment 7

5 years ago
Posted image Screenshot - wide
Assignee

Comment 8

5 years ago
Posted patch WIP: Generic buttons patch (obsolete) — Splinter Review
Attachment #8430287 - Attachment is obsolete: true
Attachment #8430939 - Flags: review?(benediktp)
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?
Assignee

Comment 10

5 years ago
Posted patch Generic buttons patch (obsolete) — Splinter Review
Attachment #8430939 - Attachment is obsolete: true
Attachment #8430939 - Flags: review?(benediktp)
Attachment #8431334 - Flags: review?(benediktp)
Assignee

Comment 11

5 years ago
(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 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)
Attachment #8431334 - Flags: feedback+
Assignee

Updated

5 years ago
Blocks: 1018060
Assignee

Comment 13

5 years ago
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).
(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.
(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.
Assignee

Comment 16

5 years ago
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.
Assignee: nobody → mayanktg
Attachment #8431334 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8431334 - Flags: review?(benediktp)
Attachment #8445387 - Flags: feedback?(florian)
Assignee

Comment 17

5 years ago
Attachment #8445387 - Attachment is obsolete: true
Attachment #8445387 - Flags: feedback?(florian)
Attachment #8445454 - Flags: feedback?(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.
Attachment #8445454 - Flags: feedback?(florian) → feedback+
Assignee

Comment 20

5 years ago
(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.
Assignee

Comment 21

5 years ago
Posted patch v4: Generic buttons patch. (obsolete) — Splinter Review
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.
Attachment #8445454 - Attachment is obsolete: true
Attachment #8457477 - Flags: review?(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?
Attachment #8457477 - Flags: review?(florian) → review-
Assignee

Comment 23

5 years ago
Posted patch v5: Generic buttons patch. (obsolete) — Splinter Review
Changests:
* removed separate border-bottom for displayName and convToolbarbuttonBox.
* removed class convToolbarbutton
* fixed the CSS when the window is less than 250px.
Attachment #8457477 - Attachment is obsolete: true
Attachment #8458138 - Flags: review?(florian)
Assignee

Comment 24

5 years ago
Sorry for the ugly formatting.
Assignee

Comment 25

5 years ago
(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 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.
Attachment #8458138 - Flags: review?(florian) → review-
Assignee

Comment 27

5 years ago
Posted patch v6: Generic buttons patch. (obsolete) — Splinter Review
Attachment #8458138 - Attachment is obsolete: true
Attachment #8463556 - Flags: review?(florian)
Assignee

Comment 28

5 years ago
(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.
(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.
Assignee

Comment 30

5 years ago
Posted patch v7: Generic buttons patch. (obsolete) — Splinter Review
Attachment #8463556 - Attachment is obsolete: true
Attachment #8463556 - Flags: review?(florian)
Attachment #8463561 - Flags: review?(florian)

Updated

5 years ago
Attachment #8463561 - Flags: feedback?(clokep)
Posted patch CSS tweaks (obsolete) — Splinter Review
I tested this on Mac and tweaked the CSS until things looked good.

Updated

5 years ago
Attachment #8498510 - Flags: review?(aleth)

Updated

5 years ago
Attachment #8463561 - Attachment is obsolete: true
Attachment #8463561 - Flags: review?(florian)
Attachment #8463561 - Flags: feedback?(clokep)
Comment on attachment 8498510 [details] [diff] [review]
Patch v8

Would still be nice to have someone testing this on Windows.
Attachment #8498510 - Flags: feedback? → feedback?(clokep)

Comment 34

5 years ago
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?
Attachment #8498510 - Flags: review?(aleth) → review+
Attachment #8498505 - Attachment is obsolete: true
http://hg.mozilla.org/comm-central/rev/57a529a4633e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Blocks: 954310
Attachment #8498510 - Flags: feedback?(clokep)
Depends on: 1097995
You need to log in before you can comment on or make changes to this bug.