Closed
Bug 1004930
Opened 11 years ago
Closed 10 years ago
Generic way to add buttons for actions to a conversation
Categories
(Instantbird Graveyard :: Conversation, enhancement)
Instantbird Graveyard
Conversation
Tracking
(Not tracked)
RESOLVED
FIXED
1.6
People
(Reporter: benediktp, Assigned: mayanktg)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 11 obsolete files)
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•11 years ago
|
||
(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•11 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•11 years ago
|
||
Comment 4•11 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•11 years ago
|
||
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 6•11 years ago
|
||
Assignee | ||
Comment 7•11 years ago
|
||
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #8430287 -
Attachment is obsolete: true
Attachment #8430939 -
Flags: review?(benediktp)
Comment 9•11 years ago
|
||
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•11 years ago
|
||
Attachment #8430939 -
Attachment is obsolete: true
Attachment #8430939 -
Flags: review?(benediktp)
Attachment #8431334 -
Flags: review?(benediktp)
Assignee | ||
Comment 11•11 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 12•10 years ago
|
||
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 | ||
Comment 13•10 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).
Comment 14•10 years ago
|
||
(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•10 years ago
|
||
(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•10 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•10 years ago
|
||
Attachment #8445387 -
Attachment is obsolete: true
Attachment #8445387 -
Flags: feedback?(florian)
Attachment #8445454 -
Flags: feedback?(florian)
Assignee | ||
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
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•10 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•10 years ago
|
||
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 22•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
Sorry for the ugly formatting.
Assignee | ||
Comment 25•10 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 26•10 years ago
|
||
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•10 years ago
|
||
Attachment #8458138 -
Attachment is obsolete: true
Attachment #8463556 -
Flags: review?(florian)
Assignee | ||
Comment 28•10 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.
Comment 29•10 years ago
|
||
(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•10 years ago
|
||
Attachment #8463556 -
Attachment is obsolete: true
Attachment #8463556 -
Flags: review?(florian)
Attachment #8463561 -
Flags: review?(florian)
Updated•10 years ago
|
Attachment #8463561 -
Flags: feedback?(clokep)
Comment 31•10 years ago
|
||
I tested this on Mac and tweaked the CSS until things looked good.
Comment 32•10 years ago
|
||
Attachment #8498510 -
Flags: feedback?
Updated•10 years ago
|
Attachment #8498510 -
Flags: review?(aleth)
Updated•10 years ago
|
Attachment #8463561 -
Attachment is obsolete: true
Attachment #8463561 -
Flags: review?(florian)
Attachment #8463561 -
Flags: feedback?(clokep)
Comment 33•10 years ago
|
||
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•10 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+
Updated•10 years ago
|
Attachment #8498505 -
Attachment is obsolete: true
Comment 35•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Comment 36•10 years ago
|
||
Updated•10 years ago
|
Attachment #8498510 -
Flags: feedback?(clokep)
You need to log in
before you can comment on or make changes to this bug.
Description
•