Closed Bug 1597131 Opened 2 months ago Closed 2 months ago

Port bug 1597120 to Thunderbird - Remove XUL mousethrough attribute

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 72.0

People

(Reporter: mkmelin, Assigned: mkmelin)

References

Details

Attachments

(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
Status: NEW → ASSIGNED
Attachment #9109402 - Flags: review?(paul)
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.
Attachment #9109402 - Flags: review?(paul)
Attachment #9109402 - Flags: review-
Attachment #9109402 - Flags: feedback+
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 {
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]
bug1597131_rm_mousethrough.patch

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

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

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

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6ce35172d662
Port bug 1597120: Remove use of XUL mousethrough attribute. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 2 months 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":
https://hg.mozilla.org/comm-central/log?rev=port
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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 mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/e370eb044bf9
followup to make chat tooltips work again. r=khusil

Status: REOPENED → RESOLVED
Closed: 2 months ago2 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.