896216 - Simplify message collapsing

Status: RESOLVED WORKSFORME

(Thunderbird :: Instant Messaging, defect)

Description

[:Mook]

Attachment: simplify JS

See attached patch; this simplifies the message (event) collapsing code to be mostly driven from CSS.  Looks the same, but less code.

You'll probably want to just ignore the old code and read the new stuff - this patch was generated on top of the one from bug 760762.

Comment 1

[Florian Quèze [:florian]]

Comment on attachment 778889 [details] [diff] [review]
simplify JS

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

Looks great overall, I'm looking forward to this simplification :). I would like to have another look on the updated patch, but all the requested changes are trivial.

::: mail/components/im/messages/Footer.html
@@ +6,3 @@
>  function groupEvents(parent)
>  {
> +  if (parent.querySelector(".button.hide")) {

No { here, there's only one line inside it :).

@@ +7,5 @@
>  {
> +  if (parent.querySelector(".button.hide")) {
> +    return; // already grouped
> +  }
> +  if (!parent.querySelector("p.event:nth-of-type(4)")) {

Same here.

@@ +21,4 @@
>  
> +function toggle_groupEvents(aEvent)
> +{
> +  var target = aEvent.target;

The target intermediary variable isn't needed any more.

@@ +22,5 @@
> +function toggle_groupEvents(aEvent)
> +{
> +  var target = aEvent.target;
> +  target.parentNode.classList.toggle("hide-children");
> +}

And this function is now so simple (one line!) that I would likely just inline it directly as
button.addEventListener("click", function(aEvent) {
  ...
});

@@ +34,3 @@
>  
> +      if (node.tagName == "P" && node.classList.contains("event"))
> +        groupEvents(node.parentNode);

We may want to return early here, and inline the groupEvents function (it's simple too :)). Not sure.

@@ +38,4 @@
>    }
>  }
>  
> +MutationObserver(checkNewText).observe(

Is checkNewText still a good name? It seems to only check for events/system messages.

@@ +38,5 @@
>    }
>  }
>  
> +MutationObserver(checkNewText).observe(
> +  document.getElementById("ibcontent"), {childList:true, subtree:true});

Space after ':'.

::: mail/components/im/messages/main.css
@@ +115,1 @@
>    background: url('Bitmaps/minus-hover.png') no-repeat left center;

I think you can just do:
background-image: url(...);
and the no-repeat left center will be kept from the .button.hide background rule.

@@ +129,5 @@
> +  color: GrayText;
> +}
> +
> +.hide-children > p.event:not(:first-of-type):not(:last-of-type) {
> +	display: none;

Fix the indent please.

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 778889 [details] [diff] [review]
simplify JS

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

::: mail/components/im/messages/Footer.html
@@ +29,5 @@
> +{
> +  for (let mutation of aMutations) {
> +    for (let node of mutation.addedNodes) {
> +      if (!(node instanceof HTMLElement))
> +        return;

Did you mean continue; instead of return;?

Comment 3

[:Mook]

Attachment: Based on tip, address comments

(In reply to Florian Quèze [:florian] [:flo] from comment #1)

The new patch is just based on the tip, no previous patches needed.

(Comments not in the reply are addressed in the patch, they're uninteresting)

> ::: mail/components/im/messages/Footer.html
> @@ +22,5 @@
> > +function toggle_groupEvents(aEvent)

> And this function is now so simple (one line!) that I would likely just
> inline it directly as
> button.addEventListener("click", function(aEvent) {
>   ...
> });
>
Inlined.

> @@ +34,3 @@
> >  
> > +      if (node.tagName == "P" && node.classList.contains("event"))
> > +        groupEvents(node.parentNode);
> 
> We may want to return early here, and inline the groupEvents function (it's
> simple too :)). Not sure.
> 
See below; it's meant to be a continue above, not a return.  So we can't early return.  Inlined; it actually did end up simple enough that the readability wasn't affected that much.

> @@ +38,4 @@
> >    }
> >  }
> >  
> > +MutationObserver(checkNewText).observe(
> 
> Is checkNewText still a good name? It seems to only check for events/system
> messages.
> 
I thought of it as "check that occurs when new text appears", rather than "check that what was inserted was new text" - it's all new text, system messages or not!  I can call it checkForEvents, but that would confuse me into thinking it's about DOM events.  checkForSystemMessages is better, but 1) long, and 2) is confusing because it's not referred to as system messages in the CSS class name.

Comment 4

[aleth [:aleth]]

It would be interesting to know if these changes also bring a performance win (using the profiler).

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 793292 [details] [diff] [review]
Based on tip, address comments

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

One detail that can be further simplified; looking great otherwise.

::: mail/components/im/messages/Footer.html
@@ +23,4 @@
>  
> +      // Getting here means it's an element we want to group.
> +      var button = document.createElement("p");
> +      button.setAttribute("class", "button hide");

You always use the .button and .hide classes together, and there's no longer a .show class, so I think you should use only one. When I ported your patch to Instantbird (https://bugzilla.instantbird.org/show_bug.cgi?id=2126) I named it .eventToggle, so I think it would be nice to be consistent.

Comment 6

[:Mook]

Attachment: use .eventToggle

Also tried to look for easy sync opportunities to sync with instantbird; didn't touch the actual margin etc sizes, though, just the ordering.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 795819 [details] [diff] [review]
use .eventToggle

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

The only point I really care about is the last one about hr. The other 2 I probably wouldn't even have mentioned if I didn't have something more 'real' to comment on.

::: mail/components/im/messages/Footer.html
@@ +17,2 @@
>  
> +      if (parent.querySelector(".eventToggle"))

If you want to really match Instantbird, we removed the querySelector here and used if (parent.grouped)

@@ +23,4 @@
>  
> +      // Getting here means it's an element we want to group.
> +      let button = document.createElement("p");
> +      button.classList.add("eventToggle");

Any reason for not using button.className = here?

@@ +27,5 @@
> +      button.addEventListener("click", function (event)
> +        event.target.parentNode.classList.toggle("hide-children"));
> +      parent.insertBefore(button,
> +                          parent.querySelector("p.event:nth-of-type(2)"));
> +      parent.classList.add("hide-children");

Here in Instantbird we added: parent.grouped = true;

::: mail/components/im/messages/main.css
@@ +126,5 @@
> +  content: "\2026"; /* … */
> +  color: GrayText;
> +}
> +
> +.hide-children > :-moz-any(p.event, hr):not(:first-of-type):not(:last-of-type) {

Why are you now hiding hr tags too? They already have display: none !important; in main.css.

By the way, would it make any visible change if we just removed <hr/> from NextStatus.html and the hr rule from main.css?

I think the only reason why we haven't removed them yet is that we were afraid doing so would break the JS code that you are removing anyway ;).

Comment 8

[:Mook]

Attachment: once more, with no hr

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> ::: mail/components/im/messages/Footer.html
> @@ +17,2 @@
> >  
> > +      if (parent.querySelector(".eventToggle"))
> 
> If you want to really match Instantbird, we removed the querySelector here
> and used if (parent.grouped)
Yep, gave up on that :p

> @@ +23,4 @@
> >  
> > +      // Getting here means it's an element we want to group.
> > +      let button = document.createElement("p");
> > +      button.classList.add("eventToggle");
> 
> Any reason for not using button.className = here?
Mainly in the case of somebody wanting to be crazy and have external scripts poking into this thing.  This way, we won't clobber any existing classes people add.

> @@ +27,5 @@
> > +      button.addEventListener("click", function (event)
> > +        event.target.parentNode.classList.toggle("hide-children"));
> > +      parent.insertBefore(button,
> > +                          parent.querySelector("p.event:nth-of-type(2)"));
> > +      parent.classList.add("hide-children");
> 
> Here in Instantbird we added: parent.grouped = true;
Didn't bother :p  (Mostly because I don't like expando properties in documents I only kind of control)

> ::: mail/components/im/messages/main.css
> @@ +126,5 @@
> > +  content: "\2026"; /* … */
> > +  color: GrayText;
> > +}
> > +
> > +.hide-children > :-moz-any(p.event, hr):not(:first-of-type):not(:last-of-type) {
> 
> Why are you now hiding hr tags too? They already have display: none
> !important; in main.css.
Oh, that was just to sync with your patch; since that didn't seem interesting, I reverted that hr bit.

> By the way, would it make any visible change if we just removed <hr/> from
> NextStatus.html and the hr rule from main.css?
> 
> I think the only reason why we haven't removed them yet is that we were
> afraid doing so would break the JS code that you are removing anyway ;).
Nope; I killed the <hr> instead.  Looks about the same.  Since this will miss the 24 train (I hope!) I wasn't too worried about fallout - we'd have months to check, and I'd see it ;)

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 797080 [details] [diff] [review]
once more, with no hr

(In reply to :Mook from comment #8)

> > > +      // Getting here means it's an element we want to group.
> > > +      let button = document.createElement("p");
> > > +      button.classList.add("eventToggle");
> > 
> > Any reason for not using button.className = here?
> Mainly in the case of somebody wanting to be crazy and have external scripts
> poking into this thing.  This way, we won't clobber any existing classes
> people add.

I'm curious, how can people add classes to a "p" element we have just created but not inserted in the document yet?


> > Why are you now hiding hr tags too? They already have display: none
> > !important; in main.css.
> Oh, that was just to sync with your patch; since that didn't seem
> interesting, I reverted that hr bit.
> 
> > By the way, would it make any visible change if we just removed <hr/> from
> > NextStatus.html and the hr rule from main.css?
> > 
> > I think the only reason why we haven't removed them yet is that we were
> > afraid doing so would break the JS code that you are removing anyway ;).
> Nope; I killed the <hr> instead.

I'm all for killing the <hr>, but then its CSS rule in main.css should be killed too.

Comment 10

[aleth [:aleth]]

A fix for this was ported in Bug 966245.