622490 - Make commit message tooltip elements permanently part of the DOM.

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, enhancement)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

It would be nice if the tooltips that are generated when I hover over a long commit message were permanently part of the page's DOM.

I'm trying to write an extension that adds the ability to take an entire push's changes and add all of the commit information to the system's clipboard as nicely formatted text. 

When the user right-clicks on a changeset on TBPL, the addon receives the node that contains all of the commits from that particular push as a starting reference. It then iterates through all of the individual changesets from the push, parsing out the data from the raw HTML, then copies all of the information to the clipboard.

That all works fine, as long as the user didn't right-click on the tooltip that gets generated.

When the user right-clicks on the tooltip, the addon receives one of the <span> elements in the tooltip's <div> container as the starting reference. Because of where the tooltip is added to the DOM, the addon would have to work its way up the DOM to get to the node that it usually starts with and then proceed from there.

But the tooltip actually gets removed from the DOM before the addon gets a chance to work its way up, so it can only get up to the tooltip's <div>. Anything further up than that is null, so the addon can't find any information other than the particular changeset that was originally selected.

I guess this is a roundabout way of asking whether it'd be possible for TBPL in the future to have the tooltips be automatically generated and stored when the page loads, so that it's just a matter of using CSS to show/hide the tooltip on hover, instead of being generated/deleted on the fly.

Comment 1

[Arpad Borsos [:Swatinem]]

I thought about changing the tooltip animation to css transitions and thus cutting down on a bit of code, also to avoid adding an additional dom node.
I will take a look into it.

Comment 2

[Arpad Borsos [:Swatinem]]

Attachment: patch

How about this? Unfortunately, I couldn’t get rid of the cloned div, transitions don’t interact well with overflow and not explicitly defined width. As I think about it, it may be even better; we don’t want the element to grow while transitioning, that would definitely look ugly.

Comment 3

[Arpad Borsos [:Swatinem]]

Comment on attachment 500830 [details] [diff] [review]
patch

Hm using opacity is not a good solution here, you :hover the element when you are mousing over the results.

Comment 4

[Arpad Borsos [:Swatinem]]

Attachment: patch

Using a delayed but unanimated margin-left to hide the element away is working fine.
Happy birthday Markus :-)

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 512128 [details] [diff] [review]
patch

(In reply to comment #4)
> Happy birthday Markus :-)

Thank you! :-)

This works way better than I had expected it to. Great stuff!

>Bug 622490 - Make commit message tooltip elements permanently part of the DOM; r=mstange

"and use CSS transitions instead of JavaScript for the animations"

>diff --git a/css/style.css b/css/style.css

> .patches > li > div {
>   white-space: nowrap;
>   margin-left: 95px;
> }
>-.patches > li > .popup.hovering {
>+.patches > li > .popup {

>   padding: 2px 5px;
>-  margin: -2px -5px;
>+  margin: -2px 55px -2px -9000px;

>+  -moz-transition-property: opacity, margin-left;
>+  -moz-transition-duration: 200ms, 0ms;
>+  -moz-transition-delay: 300ms, 500ms;

>+}
>+.patches > li:hover > .popup {
>+  opacity: 1;
>+  margin-left: 90px;

The new coupling here between the 95px, the 5px and this margin-left is a bit unfortunate. Couldn't you keep the styling on the span and transition between -9000px and -5px? I guess it doesn't matter much anyway. But if you don't style the span any more you can probably remove it.

>+  -moz-transition-property: opacity, margin-left;
>+  -moz-transition-duration: 200ms, 0ms;
>+  -moz-transition-delay: 500ms;

Only transition-delay actually changes, so I think the other two properties can be removed.

Comment 6

[Arpad Borsos [:Swatinem]]

(In reply to comment #5)
> The new coupling here between the 95px, the 5px and this margin-left is a bit
> unfortunate. Couldn't you keep the styling on the span and transition between
> -9000px and -5px? I guess it doesn't matter much anyway. But if you don't style
> the span any more you can probably remove it.

Moved the styling to the span, works fine.

> >+  -moz-transition-property: opacity, margin-left;
> >+  -moz-transition-duration: 200ms, 0ms;
> >+  -moz-transition-delay: 500ms;
> 
> Only transition-delay actually changes, so I think the other two properties can
> be removed.

You are right. Why did I remember that one needed to specify both the forth and back transition?

Also note:
+        if (div.width() - div.children().width() > 0)
Now that the padding is on the span.

Pushed as http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/351de85e07af

Comment 7

[Markus Stange [:mstange]]

(In reply to comment #6)
> Also note:
> +        if (div.width() - div.children().width() > 0)
> Now that the padding is on the span.

The 10 was completely on purpose (the styling was on the span before, too): When the text fills the available space almost but not completely, the user can't easily tell whether the text overflows, so he might hover over the text to check whether he's missing anything. I've found the experience of a yellow box that says "you're not missing anything" to be more satisfactory than simply ignoring the mouseover and keeping the user waiting until he realizes that there's no hidden text.

It's a subtle and non-obvious tweak, but I think it's the right thing to do :-)

I've restored it: http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/30fe0b271294

Comment 8

[Arpad Borsos [:Swatinem]]

Alright. I just had a case where I thought that there should definitely be no popup whereas there was one.