mentions: Fix highlight extending unproportionately.

[ishammahajan]

Mentions highlighting extends to the bottom and not at all at the
top. This commit fixes that, by some CSS which might be confusing to
a reader.

With .message + .message we change the top and bottom padding of
messages which come directly after messages. This causes the spacing
to get messed up in the case where a header comes right after a
message which succeeds another message (so message --> message -->
header), it becomes 0.5rem instead of the default/previous 1rem.

.message + .message + .header selector fixes this issue by
introducing a top-margin to such a header. This method appears to be
hacky but this is the cleanest way such an issue could be fixed
without a huge change in the entire CSS.

Fixes #3513.

[gnprice]

Interesting -- thanks for the investigation and fix!

• One question I had looking at this code was: what are the values that these are overriding? Is this adding more padding between messages?

  Turns out:

  • For .message-brief, the vertical padding is 0px and 1px;
  • ... but otherwise for .message, it's 1px and 1px.

  So for .message-brief, this moves the content around in its box; but for other .message (i.e. for sender-showing messages), it reduces the total padding.

  It's possible this is a good thing! I'd never quite noticed this, but there's kind of a lot of vertical whitespace between consecutive messages from different senders. Here's screenshots -- before:

  

  then after:

  

  (These are on different emulated devices with different screen widths, so there's some unrelated variation in the text wrapping -- but the vertical spacing is all from the code change.)

  If we change that it should be intentional, though. In particular we'll definitely want to highlight it in the commit message -- that is actually a more significant change than Highlight when mentioned extends to the bottom disproportionately. #3513 , because it's much more common.

• So this reduces the padding-bottom on .message + .message (from 1rem to 0.5rem); then adds back 0.5em of padding at the top of a .header that follows one of those.

  But a .header (or another .message) isn't the only thing that can be after some .message`s. Other possibilities include:

  • a timerow -- and indeed this does reduce the spacing in that case, with before:
    
    and after:
    

  • the end of the message list -- ditto

  • typing notifications

  • the "loading" spinner

  I don't think we'll want to chase all of those with further CSS rules.

  Again, it's possible this change is an improvement in those cases! We should just do it consciously, not by accident.

• Oh hmm in fact... then there are the messages that aren't after another .message -- they're after a timerow or a recipient header.

  This doesn't affect them, so they still have 1rem of bottom padding... but if there's another message just after them, that one grows 0.5rem of top padding. That makes 1.5rem of total padding. And the extra space can indeed look pretty wrong:

  

  That seems also much more common than Highlight when mentioned extends to the bottom disproportionately. #3513.

---

I think in short the .message + .message trick, nifty though that CSS feature is 😉 , is probably not going to be the right approach here.

Can you find another solution? If it ends up involving some changes to the HTML we generate, that's fine.

[ishammahajan]

@gnprice thanks for the extensive investigation on this!

• it reduces the total padding

That was definitely not my intention when making this PR (I did not consider a case where a message-full came right after another message-full), but it looks like that actually stabilises the spacing between two different messages in the sense that all messages would now have spacing between them to be a total of 1rem.

In this case I think I'll be making two PRs one where this change doesn't occur (basically fixing this issue, making it match current design) and another where we make this change with a commit of its own.

If it ends up involving some changes to the HTML we generate, that's fine.

Well, that solution looks very dirty (essentially inserting dummy elements just to increase/decrease padding).

Pushed a new version, check it out!

[gnprice]

Thanks @ishammahajan !

This looks like a plausible direction for the code, and it looks right visually in the quick example I just looked at. I don't feel I 100% yet understand what it's doing, though.

I quite appreciate the idea of separating into a commit that makes one change and holds other effects constant, and another commit that makes another change -- that's quite helpful.

Here's an idea that I think would make it easier to understand the first change:

• As is, it does some refactoring of which rules exist where and what they apply to, and also changes some values.
• Can you split it into (a) first a commit that rearranges what rules exist to match the state you want, but keeps all the CSS property values the same, and then (b) a commit that changes the property values to the ones you want to use?
  • Or if it's hard to completely do that, then do that as far as it makes sense to do. For example, both before and after the current version there's a rule on .message-brief that sets padding-left to 64px, but they're in different places and take different shapes (shorthand vs. not), so the reader has to notice that and mentally block it out to see the things that have substantively changed.
  • I think commit (b) in this scheme will be changing a 0 and 16px to 8px and 8px... and adding the margin... and I think another thing or two, but I'm not 100% sure what. So that's why I'm interested in seeing it written out. 😄

Also:

• The commit message on the first commit says it "sneak fixes" a certain issue -- but from the code, the second commit message, and your comment here, I think you then decided (and I agree 😉 ) that it'd be better to keep that behavior unchanged in the first commit and fix it explicitly in a separate commit. So please update the commit message to reflect that; or otherwise clarify, if I've misunderstood.

[ishammahajan]

• Can you split it into (a) first a commit that rearranges what rules exist to match the state you want, but keeps all the CSS property values the same, and then (b) a commit that changes the property values to the ones you want to use?

Makes sense, done!

In commit 1, the body > .message rule is more specific than it needs to be, because it is more readable this way, and a connection with the rule above body > *:not(.message) (which is essential) is apparent.

• The commit message on the first commit says it "sneak fixes" a certain issue -- but from the code, the second commit message, and your comment here

I understand the confusion. Edited the commit message to make things clearer! Basically, there are two 'bugs' which come with fixing #3513. One (as mentioned in commit 2's message, in the last paragraph) makes it so that in some cases timerows (date pills) don't have as much padding according to previous design as they should have; this has been 'fixed' in commit 2 itself -- has not been dealt with, because I found it very difficult to revert the design to the version prior to this commit.

Second issue (again mentioned in the commit message for commit 2 -- rule 4) is where a message-full would come after a message-brief, there was a loss of 24px of padding-top. This I managed to revert back to the previous design using another rule, which I removed in commit 3.

[gnprice]

Thanks @ishammahajan -- this is helpful!

A couple more questions, which I'm now better able to ask 🙂 :

+body > .message {
+  padding: 16px;
+}
+body > .message-brief {
+  padding-left: 0 16px 16px 64px;
+}

• I think you mean padding: 0 16px 16px 64px;, right? That's what the old code has, and also that's syntactically valid CSS 😉

• In every case where the second rule applies, the first one does too, right? And this rule is overriding it because it comes later. Then in the second commit:

 body > .message {
-  padding: 16px;
+  padding: 8px 16px;
 }
 body > .message-brief {
-  padding-left: 0 16px 16px 64px;
+  padding-left: 64px;
+}
+.message + .message-full {
+  padding-top: 24px;
 }

you change the values in the first rule, and also change what set of them gets overridden in the second rule; and also start overriding one of those for certain .message-full instead, in the third rule.

• I think every .message is either a .message-brief or a .message-full. Is that right? If so, I think it'd make this easier to understand if the .message-full and .message-brief padding were each spelled out in full, in separate rules, rather than have one of them overriding the other. The first commit would be a good place for that, or perhaps a new second pure-refactor commit inserted before the main commit.

  And if I'm missing something and there's another kind of .message, then I'm extra interested in having more of it explained 🙂

[gnprice]

+body > *:not(.message) {
+  margin: 8px 0;
+}

• I think the intention here is to add 8px of vertical margin to the elements that sit between messages as their siblings, and participate in the vertical layout with the messages, but aren't messages. (I.e., what the visible-messages code in js.js calls "message-peers".) Is that right?

• Is it definitely the case that you want this for all message-peers? I worry that that's a bit of an unbounded list -- can you name all of the different things this applies to off the top of your head, without forgetting some? I'm not particularly confident I can. 😉

  In particular, I wonder if there might be some message-peers that have more margin than this, and this would reduce the margin. But there might also be others that we don't actually want to add this margin to.

  Also, this applies to all the body's non-message children, including stuff like the error banners and scroll-to-bottom button that don't participate in the messages' layout. We probably don't want it on them.

• I think it'd likely be better to list the things this is intended to apply to. Is that just the date-pill containers and the recipient headers, or are there others?

[ishammahajan]

Thanks for the swift review!

• I think you mean padding: 0 16px 16px 64px;, right? That's what the old code has, and also that's syntactically valid CSS 😉

That's right, my bad 🙂.

• I think it'd make this easier to understand if the .message-full and .message-brief padding were each spelled out in full, in separate rules, rather than have one of them overriding the other.

Sorry, this just seemed like good syntactic sugar 😛. I have made it more explicit.

• I think the intention here is to add 8px of vertical margin to the elements that sit between messages as their siblings, and participate in the vertical layout with the messages, but aren't messages. (I.e., what the visible-messages code in js.js calls "message-peers".) Is that right?

Yup, that's right. This includes timerows, headers, typing, scroll-bottom, spinner-newer and others.

• Also, this applies to all the body's non-message children, including stuff like the error banners and scroll-to-bottom button that don't participate in the messages' layout. We probably don't want it on them.

I can't recall all of them. Unfortunately in this case too, I was enticed by the syntactical sugar. I think my intension in this case was to give this margin to every timerow and every header. Fixed.

[gnprice]

Thanks, this is helpful!

It still doesn't quite give the kind of reasoning that I'd want to go through in order to convince myself that I understand exactly what the main commit here is doing -- or, what's almost the same thing, that it doesn't cause any changes at all except the ones the commit message describes.

So, I'll try to go through that kind of reasoning here. I'll be sort of thinking aloud. First, a version where I'm problem-solving trying to reconstruct a change like this from scratch:

• The thing we're trying to accomplish is to change a .message-brief from having padding-vertical: 0 16px to padding-vertical: 8px 8px. We're moving 8px of padding from the bottom to the top. Or in other words, we're keeping the size of its padding box, but moving the content down by 8px inside the padding box.

• To try to keep everything else looking the same, we basically want to move all the other things around it down by 8px within their own padding boxes (taking away 8px from the bottom to give to the neighbor below, and adding 8px at the top taken from the neighbor above)... as long as everything's padding is visually indistinguishable from each other's, anyway.

• OK, so for .message-full we had padding-vertical: 16px 16px, and we can change that to 24px 8px. A little lopsided, may need to revisit that, but let's say that for now.

• What else do messages (of either kind) sit next to? Hmm... timerows and recipient-headers; and a few miscellaneous things at the very top and bottom.

• Timerows have 8px 8px of vertical padding. OK, we can make it 16px 0. But then:

• ... Recipient headers. They have padding which is with their own distinct background -- it can't just be exchanged invisibly with its neighbors' padding. And they don't have any margin which could be used instead.

  • On the top, we could add 8px of margin, to replace the upper neighbor's lost 8px of padding.

  • But on the bottom, there's nothing to take to give to the neighbor below. We have to make that neighbor not increase its padding.

• OK, well, the neighbor under a recipient header is always a .message-full -- timerows go above, and there's always a .message-full before any .message-brief. So we can have a .header + .message-full keep 16px of padding-top, instead of growing there to 24px.

• Then I think that takes care of everything among the messages, recipient headers, and timerows. The only loose end is the loading and typing indicators etc. at top and bottom.

---

Now, a version looking at the actual commit and trying to validate that it doesn't have any unexpected behavior. (This is something I'd do after writing a commit myself, just like I do when reviewing one someone else wrote.)

• The goal is that change of .message-brief's padding-vertical from 0 16px to a balanced 8px 8px, as mentioned above. OK, it does that, cool.

• That leaves the question of a .message-brief element's neighbors. OK, the change in the body > .message-full rule takes away 8px at bottom, as needed when that's the upper neighbor... and the .message + .message-full rule adds 8px at top, as needed when that's the lower neighbor, either of a .message-brief or a .message-full.

• Great, so: all .message elements lose 8px below; and all gain 8px above, if they have another .message there. That all matches up.

• But, not yet matched up:

  • any non-message under a message needs to gain 8px above;
  • any non-message above a .message-brief needs to lose 8px below;
  • and hmm, any non-message above a .message-full needs to gain 8px below, because we actually reduced the padding-top for a .message-full in that case. Huh, that's interesting.

• Meanwhile: the one remaining change that's here in this commit and that we haven't yet analyzed: we added margin-vertical: 8px to .timerow and also .header.

  • (At least I'm guessing that this is intended to add to .margin-vertical and to not change .margin-horizontal. Hmm, OK, and from inspecting the status quo in Chrome DevTools, there is in fact no margin on those elements, so that's confirmed.)

• Well, that takes care of the two cases that need to gain 8px. It causes its own changes when those non-messages meet any other non-messages.

• Ohhh, I guess "non-message above a .message-brief" is a thing that can't happen. OK, that part's good, then.

• OK, so at this point, changes not matched up:

  • For any non-message next to a .timerow or .header, we've added 8px of margin.
  • For any non-message, non-timerow-or-header next to a message (on either side), we've removed 8px of padding.
  • (Also the change that was the goal in the first place: we've moved the padding on a .message-brief to be balanced, which matters when the message is highlighted due to a mention. Plus we've moved it on certain .message-full elements to be unbalanced, but at least it's nonzero on both sides.)

• IOW: the total effect of the change seems to be the goal, and two side effects.

• How does that compare with what the commit message describes?

  • "Mentions highlighting extends to the bottom and not at all at the top. This commit fixes that" -- great, this matches the last bullet above, though a bit indirectly.

  • Then there's some text that just says the same thing as the code. Hmm actually the same as a previous version of the code.

  • OK, then: item 4 in the list says items 1-3 would change two things in the design; one is .message + .message-full, and item 4 is about cancelling it out, for net no change.

  • Then the second behavior change the commit message describes (i.e., the only one other than the goal, i.e. the only side effect):

    Second, which should not be fixed is where the distance between a timerow and a message which precedes or succeeds it was 1.5rem (24px), making the total empty space because of it (if it happened to be in between two messages) 3rem (48px)! This should not be the case design wise and this commit essentially sneak fixes that.

    Hmm. I think this is saying there was too much space in this case? And that this commit has the side effect of making there be less space, by some unspecified amount.

• Hmm, the one side effect in the commit message doesn't seem to line up with either of the side effects we found above.

  • What happens in practice? ... OK, checking with Chrome DevTools: before, we have 16px of padding-bottom, then 8px of padding-top, then the date, then 8px of padding-bottom, then 16px of padding-top.
  • And after, we have 8px of padding-bottom, then 8px of margin-top, then 8px of padding-top, then the date; then the same things in reverse.
  • So 24px total space above and below, both before and after. Seems like this side effect doesn't actually happen. Unless I misunderstood the description?

• Meanwhile, there are those two other side effects. What about them?

  • Timerow above header: yup, was 8px, now 16px (padding, then margin). The two margins get collapsed, though, which is why it isn't 24px.
    • Not sure if I prefer the extra space, or not.
  • That's the only case of a timerow-or-header next to another timerow-or-header. So the remaining side effects we deduced are with the miscellaneous things at the very top and bottom.
  • At the very top, there's always a timerow; we've added 8px of margin.
  • At the very bottom, there's always a message; we've removed 8px of padding.
  • Looking at the very bottom... yup, reduced padding confirmed. It looks probably good, in the common case where there's no miscellaneous widget there and it's just the end of the message list.
  • Looking at the very top... yup, extra margin confirmed. It doesn't look good, I think. (Again in that common case.)
  • I haven't tried to check whether the side effects look good or bad when there's a typing/loading indicator etc.

---

• OK, so: the effects are not as described. At a minimum we definitely want to accurately describe the effects.

• Also it would be great to write the code so that the reasoning to validate what it does can be shorter than what's above. 😁

• Also, none of the side effects of this change seem particularly bad, but as part of accomplishing the two points above it might be helpful to eliminate some of them.

[ishammahajan]

Thanks for the review, @gnprice! The explanation above seems to be matching what was in my mind. Thanks for writing (and in turn documenting) it!

• OK, so: the effects are not as described. At a minimum we definitely want to accurately describe the effects.

If I understand correctly from the review, the following is not described correctly:

• The added padding at the top of the message list.

• The reduced padding at the very bottom of the message list.

• Timerow above header: yup, was 8px, now 16px (padding, then margin). The two margins get collapsed, though, which is why it isn't 24px.

  I don't know whether it looks good either. However, with the upcoming Make timerow persistent (WhatsApp like implementation). #3496 , it might look good, I think.

• Also it would be great to write the code so that the reasoning to validate what it does can be shorter than what's above. 😁

I tried very hard to do that. The simplest way I could write it out is this I'm afraid. It does look a little convoluted though. Perhaps a simple rearranging of the css rules might make it better? Perhaps some comments (generally not preferable, I know) would make it more understandable -- containing a link to the above very helpful long explanation?

[ishammahajan]

I was just about to go create a thread for something unrelated on #mobile. I think to test out the slight changes in design made in this PR, I'll be using it for a few days on my device and report back if the they feel very out of place.

[gnprice]

I think to test out the slight changes in design made in this PR, I'll be using it for a few days on my device and report back if the they feel very out of place.

That sounds like a good plan.

If I understand correctly from the review, the following is not described correctly:

Yep, those two, plus on the other hand this bit (from my notes above):

Hmm, the one side effect in the commit message doesn't seem to line up with either of the side effects we found above. [... with details ...]

It's been a bit since I wrote that, but from rereading quickly, I think the conclusion of that bit is that the "Second, which should not be fixed" paragraph is just not true, and so should be deleted.

There's also this bit:

Then there's some text that just says the same thing as the code. Hmm actually the same as a previous version of the code.

In general, commit messages or comments that just say the same thing as the code aren't very helpful (unless the code change is so simple that just a one-line summary amounts to saying the same thing as the code, in which case that's still helpful for a commit message.)

When it's not even quite the same, then it's definitely not helpful. 🙂

Here, I think what I was referring to when I said "the same as a previous version" is this bit:

The changes in the parent commit removes all application of padding
on the highest level of the markup (just below the `body`), and
consolidates it to the top.

because in this version it doesn't look like that's what the parent commit actually does.

[ishammahajan]

That sounds like a good plan.

Went through it for a while, and found that there is one scenario where the design is a little different compared to the previous design -- between two full-messages, specially after the recent increase in size of the avatars.

But after some time using the app, it appears normal.

because in this version it doesn't look like that's what the parent commit actually does.

Huh. I may have to reword that slightly, because in my mind what I have written perfectly describes the parent commit.

Actually, now that I look at it, that paragraph doesn't contribute to the commit message at all 👀 .

---

And done! Should be ready for review again :)