1719645 - Align calendar week view

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, enhancement)

Description

[adrien.rybarczyk]

Attachment: AlignmentFor78

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.100 Safari/537.36

Steps to reproduce:

Be in weekly view.

Actual results:

There is an offset pixel on the alignment.

Bug present in 78 and 90 but the offset is not the same.

Expected results:

The elements are aligned.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug is invalid.
If you think the bot is wrong, please reopen the bug and move it back to its prior component.

Be aware this is a production bug database used by the Mozilla community to develop Firefox, and other products.
Filing test bugs here wastes the time of all our contributors, volunteers, as well as paid employees.
If you continue to abuse bugzilla.mozilla.org your account will be disabled.

Comment 2

[Richard Marti (:Paenglab)]

This comes because the width of headerscrollbarspacer and labelscrollbarspacer are set 1px wider than the scrollbar width is. Tested on Linux and Windows.

Henry, do you know what could be wrong when the width is calculated here: https://searchfox.org/comm-central/rev/c821090d64c92a05bfc49aba6d5f0d25ad5ff52e/calendar/base/content/calendar-multiday-view.js#1934 ?

Comment 3

[Richard Marti (:Paenglab)]

I found the issue: we set a margin here https://searchfox.org/comm-central/rev/c821090d64c92a05bfc49aba6d5f0d25ad5ff52e/calendar/base/themes/common/calendar-views.css#558.
Does a function exist that includes in the calculation the margins or do we need to add this margin manually?

Comment 4

[Henry Wilkes (they/them) [:henry-x]]

(In reply to Richard Marti (:Paenglab) from comment #3)

Does a function exist that includes in the calculation the margins or do we need to add this margin manually?

getBoundingClientRect only returns the border area, you have to add the margin yourself.

This issue should get fixed as a side effect once I get round to Bug 1713130. If this isn't a huge priority to fix soon, you can add the dependency.

Comment 5

[Richard Marti (:Paenglab)]

Is Bug 1713130 planned to land on TB 91? If not, I'll fix this here.

Comment 6

[Henry Wilkes (they/them) [:henry-x]]

I've not got any specific release plan, so go ahead and fix this here

Comment 7

[Richard Marti (:Paenglab)]

Attachment: 1719645-weekView-scrollbar-width.patch

Is this okay like this? I have only basic JS knowledge. So propose a smarter way if where is one.

Comment 8

[Henry Wilkes (they/them) [:henry-x]]

Comment on attachment 9230327 [details] [diff] [review]
1719645-weekView-scrollbar-width.patch

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

::: calendar/base/content/calendar-multiday-view.js
@@ +1950,5 @@
> +      // We need to include in our calculation the 1px margin-end of the
> +      // calendar-time-bar[orient="vertical"]
> +      if (this.getAttribute("orient") == "vertical") {
> +        scrollbarSize = scrollbar - 1;
> +      }

Since `scrollboxSize` and `scrollbar` are only used once, you can tidy up by dropping them and just using a non-const `scrollbarSize`.

To be on the safer side, instead of manually reducing by 1 pixel, when you compute the scrollboxChildrenSize above, you could use

let computedStyle = window.getComputedStyle(child);
if (this.getAttribute("orient") == "vertical") {
  scrollboxChildrenSize += parseFloat(computedStyle.marginLeft);
  scrollboxChildrenSize += parseFloat(computedStyle.marginRight);
} else {
  scrollboxChildrenSize += parseFloat(computedStyle.marginTop);
  scrollboxChildrenSize += parseFloat(computedStyle.marginBottom);
}

This way, if the margin changes to some other px value, this method won't break. Some disclaimers though, the parseFloat is needed because the computedStyle will return the string value, e.g. "1px", so it will ignore the trailing "px". This means that it will break if the margin is set to a non-px value, like "3em" or "auto", and in the latter case you'll have a NaN. So to be extra safe, you could check each computed value against the regular expression /^[0-9]+(\.[0-9]+\)?px$/ first.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: 1719645-weekView-scrollbar-width.patch

I think because we do the styling in our files we don't need to check with regex.

Comment 10

[Henry Wilkes (they/them) [:henry-x]]

Comment on attachment 9230339 [details] [diff] [review]
1719645-weekView-scrollbar-width.patch

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

Looks good. I've not reviewed via splinter before (only on phabricator). I just set the review to `+` to accept it. Not sure if that was right.

Comment 11

[Richard Marti (:Paenglab)]

(In reply to Henry Wilkes [:henry] from comment #10)

I just set the review to + to accept it. Not sure if that was right.

Yes, that was right.

Comment 12

[Henry Wilkes (they/them) [:henry-x]]

Comment on attachment 9230339 [details] [diff] [review]
1719645-weekView-scrollbar-width.patch

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

Looks good. I've not reviewed via splinter before (only on phabricator). I just set the review to `+` to accept it. Not sure if that was right.

::: calendar/base/content/calendar-multiday-view.js
@@ +1939,5 @@
>        // we subtract the size of the other scrollbox children from the size of the scrollbox
>        // to calculate the size of the scrollbar.
>        let scrollboxChildrenSize = 0;
>        for (const child of this.scrollbox.children) {
> +        let computedStyle = window.getComputedStyle(child);

Maybe add a comment to explain that we expect the margin to be set to a px value, so future people will be aware of the assumption.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: 1719645-weekView-scrollbar-width.patch

Added a comment that we expect px.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fc94169f2fda
In week view set the correct scrollbar width for the spacers. r=henry