Closed Bug 1597131 Opened 1 year ago Closed 11 months ago

Port bug 1597120 to Thunderbird - Remove XUL mousethrough attribute


(Thunderbird :: General, task)

Not set


(Not tracked)

Thunderbird 72.0


(Reporter: mkmelin, Assigned: mkmelin)




(2 files, 1 obsolete file)

Port: Bug 1597120 - Remove XUL mousethrough attribute

Attached patch bug1597131_rm_mousethrough.patch (obsolete) — Splinter Review

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.)

Assignee: nobody → mkmelin+mozilla
Attachment #9109402 - Flags: review?(paul)
Comment on attachment 9109402 [details] [diff] [review]

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.
Attachment #9109402 - Flags: review?(paul)
Attachment #9109402 - Flags: review-
Attachment #9109402 - Flags: feedback+
Comment on attachment 9109402 [details] [diff] [review]

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:

.timeIndicator {

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

.multiday-column-top-box, .timeIndicator {
.multiday-column-top-box .timeIndicator {
Attachment #9109402 - Flags: review-
Attachment #9109402 - Flags: feedback+

Addressing review comments

Attachment #9109402 - Attachment is obsolete: true
Attachment #9109970 - Flags: review?(paul)
Comment on attachment 9109970 [details] [diff] [review]

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

LGTM, thanks.
Attachment #9109970 - Flags: review?(paul) → review+

Actually, does this need to wait for bug 1597120?

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

Pushed by
Port bug 1597120: Remove use of XUL mousethrough attribute. r=pmorris

Closed: 1 year ago
Resolution: --- → FIXED

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.

Target Milestone: --- → Thunderbird 72.0

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).

Well, generally it is for Thunderbird. You're the only person on the project adding the "to Thunderbird":
Are all the other people doing it badly?

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.

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.

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.

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.

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.

Attachment #9112257 - Flags: review?(khushil324)
Resolution: FIXED → ---
Comment on attachment 9112257 [details] [diff] [review]

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.
Attachment #9112257 - Flags: review?(khushil324) → review+

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.

Pushed by
followup to make chat tooltips work again. r=khusil

Closed: 1 year ago11 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.