1745619 - Replace 30 lines of dom insert, reflow, calculation js for multifield message headers separator "," with 1 line of css

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Comment 1

[alta88]

Attachment: commacalc.patch

Use :after to add a comma separator.

Comment 2

[alta88]

Attachment: singleline.patch

The singleline attribute has not been used for years to collapse multiple rows via css. This followup completes the scope for this bug.

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9254922 [details] [diff] [review]
commacalc.patch

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

Good stuff, thanks for taking care of this.

Unfortunately this breaks a bunch of tests in browser_messageHeader.js, but overall it looks good and a step in the right direction.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9255073 [details] [diff] [review]
singleline.patch

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

::: mail/base/content/widgets/mailWidgets.js
@@ +1066,5 @@
>       * @param {boolean} all - If false, show only a few addresses + "more".
>       * @return {integer} The number of addresses we have put into the list.
>       */
>      _fillAddressesNode(all) {
> +      this.emailAddresses.replaceChildren();

Nice, I didn't realize this can be used to empty out children

@@ +1147,5 @@
>  
>      /**
>       * Public method to build the DOM nodes for display, to be called after all the addresses have
>       * been added to the widget. It uses _fillAddressesNode to display at most maxLinesBeforeMore lines
> +     * of ddresses plus the (more) widget which can be clicked to reveal the rest.

Let's fix the typo

Comment 5

[alta88]

Attachment: singleline.patch

Comment 6

[alta88]

Attachment: commacalc.patch

Updated to pass tests.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9256548 [details] [diff] [review]
commacalc.patch

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

Looks good, with the small change below. r=mkmelin

::: mail/themes/shared/mail/messageHeader.css
@@ +327,5 @@
>  
> +/* Separator for multifield emailaddress/newsgroup and messageid header rows. */
> +.emailDisplayButton:not(:last-child):after,
> +.messageIdDisplayButton:not(:last-child):after {
> +  content: ",";

I'd think this should be ", " (with a space)

Comment 8

[alta88]

Spaces are not recognized here, nor will hex space work as it bleeds into the next element's text decoration. It needs to be padding/margin, which will be done in Bug 1589861 for messageIds part.

Comment 9

[Magnus Melin [:mkmelin]]

Leave-open since lots of bitrot, just landing the first patch unbitrotted now

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e18ac7fbc3bd
Remove unused attribute singleline, use repaceChildren() to clear children. r=mkmelin

Comment 11

[Magnus Melin [:mkmelin]]

Seems to cause this for debug builds: Assertion failure: !disp->IsAbsolutelyPositionedStyle() || disp->DisplayInside() != StyleDisplayInside::MozBox (This may be a frame that was previously blockified but isn't any longer! It probably needs explicit 'display:block' to preserve behavior), at /builds/worker/checkouts/gecko/layout/base/nsCSSFrameConstructor.cpp:5618

https://treeherder.mozilla.org/logviewer?job_id=362602221&repo=comm-central

Comment 12

[alta88]

Attachment: commacalc.patch

Unbitrot and change newsgroups to inline-block, please try run. I already have patches in the other bug to de xul these custom elements.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/16e1c3dab3d9
Replace 30 lines of dom insert, reflow, calculation js for multifield message headers separator "," with 1 line of css. r=mkmelin