1597131 - Port bug 1597120 to Thunderbird - Remove XUL mousethrough attribute

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

Port: Bug 1597120 - Remove XUL mousethrough attribute

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1597131_rm_mousethrough.patch

Looks like a lot of these are redundant. For at least some of the cases I converted they could potentially be redundant but I've left them as it's somewhat hard to verify.

(Regarding the functionality around dragCenter-image. All I can think it why, why, why? Dragging the date label will tick forwards in the calendar one day per 0.5s. Doesn't make much sense to me.)

Comment 2

[Paul Morris [:pmorris]]

Comment on attachment 9109402 [details] [diff] [review]
bug1597131_rm_mousethrough.patch

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

Looks good overall.  All but one removal seems fine to me from looking at the code and testing out the UI.  I think we should keep that `dragCenter-image` feature in the today pane working in this bug, and remove it in its own bug if we're going to do that.  That way we could remove all the code related to it.  (I'm not sure how much value it adds.)

::: calendar/lightning/content/messenger-overlay-sidebar.xul
@@ +1769,5 @@
>                      <label id="datevalue-label" class="dateValue"
>                             ondblclick="TodayPane.onDoubleClick(event);"
>                             onmousedown="TodayPane.onMousedown(event);"/>
>                    </hbox>
> +                  <hbox flex="1" pack="center" align="center">

I found that without the `mousethrough="always"` here the `dragCenter-image` functionality didn't work.

Comment 3

[Paul Morris [:pmorris]]

Comment on attachment 9109402 [details] [diff] [review]
bug1597131_rm_mousethrough.patch

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

Oops, this comment got lost (had it open in a separate window).

::: calendar/base/themes/common/calendar-views.css
@@ +1175,5 @@
>      min-height: 1px;
>      overflow:hidden;
>  }
>  
> +.multiday-column-top-box, .timeIndicator {

I'd suggest formatting with one selector per line, for quicker reading, like this:

.multiday-column-top-box, 
.timeIndicator {

Otherwise it's hard to see the difference between these at a glance:

.multiday-column-top-box, .timeIndicator {
.multiday-column-top-box .timeIndicator {

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: bug1597131_rm_mousethrough.patch

Addressing review comments

Comment 5

[Paul Morris [:pmorris]]

Comment on attachment 9109970 [details] [diff] [review]
bug1597131_rm_mousethrough.patch

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

LGTM, thanks.

Comment 6

[Jorg K (CEST = GMT+2)]

Actually, does this need to wait for bug 1597120?

Comment 7

[Magnus Melin [:mkmelin]]

No, bug 1597120 is just removing it. The replacements are already existing.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6ce35172d662
Port bug 1597120: Remove use of XUL mousethrough attribute. r=pmorris

Comment 9

[Jorg K (CEST = GMT+2)]

One word about the commit messages:
Port bug XXX to Thunderbird.
Well, I guess the "to Thunderbird" is not needed.

Pasting the M-C bug summary is mostly also not correct. We're not removing the mousethrough attribute, M-C is removing it, so consequently we need to remove the use of it.

Comment 10

[Magnus Melin [:mkmelin]]

I think "to Thunderbird" should be there, otherwise it might be confusing when reading pushlogs later (was it for Thunderbird, was it for SeaMonkey, what was it for).

Comment 11

[Jorg K (CEST = GMT+2)]

Well, generally it is for Thunderbird. You're the only person on the project adding the "to Thunderbird":
https://hg.mozilla.org/comm-central/log?rev=port
Are all the other people doing it badly?

Comment 12

[Magnus Melin [:mkmelin]]

Of course if you look at the code it's clear, but from the summary it's not. So yes, I'd suggest to add that in the future.

Comment 13

[Jorg K (CEST = GMT+2)]

Then get everyone to put those words into their commit messages. So far, no one but you does, so for consistency, I've been removing it where I saw it - otherwise it's totally confusing.

Comment 14

[Magnus Melin [:mkmelin]]

It's a clarification, so hard to see how it would be confusing. In the future with a single repository it's also possible you're browsing commits in a list of all checkins, and not just Thunderbird commits.

Comment 15

[Jorg K (CEST = GMT+2)]

Please read comment #13:

So far, no one but you does, so for consistency, I've been removing it where I saw it - otherwise it's totally confusing.

It's confusing if one guy does it, the rest doesn't.

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: bug1597131_fix-chat.patch

The chat part broke chat tooltips. But we can't use css to fix it so let's do smarter matching in the event handler instead.

Comment 17

[Khushil Mistry [:khushil324]]

Comment on attachment 9112257 [details] [diff] [review]
bug1597131_fix-chat.patch

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

Looks good to me and working on Trunk. r=khushil.
Do you have any idea about the context menu on richlistitems? It is still not working. We can take it up on another bug.

Comment 18

[Khushil Mistry [:khushil324]]

There is an issue bug 1592422 comment 5 on the trunk. I think It is regressed from here but I will solve it in Bug 1592422.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e370eb044bf9
followup to make chat tooltips work again. r=khusil