Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#20204 closed defect (fixed)

Windows don't drag on macOS Sierra

Reported by: arlolra Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff52-esr-will-have, tbb-usability, TorBrowserTeam201610R
Cc: macsmerf, sukhbir, bitlox Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The SDK needs bumping?

Affects Tor Messenger as well.

Child Tickets

Change History (36)

comment:1 Changed 3 years ago by mcs

Fun stuff. Here is the corresponding Firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1284859

comment:2 Changed 3 years ago by gk

Keywords: ff52esr-will-have tbb-usability added

Might be worth thinking about fixing this earlier than with Tor Browser 7.0.

comment:3 Changed 3 years ago by gk

Keywords: ff52-esr-will-have added; ff52esr-will-have removed

comment:4 Changed 3 years ago by arlolra

Oddly, I downloaded Firefox ESR 45 and did not experience this problem.

comment:5 in reply to:  4 Changed 3 years ago by mcs

Replying to arlolra:

Oddly, I downloaded Firefox ESR 45 and did not experience this problem.

That is probably because Apple added an exception for Firefox <= 48 (which should also help ESR channel releases, but Tor Browser has a different bundle ID so the exception won't help us). See:
https://bugzilla.mozilla.org/show_bug.cgi?id=1284859#c11

Backporting the patches from the following bug should fix this problem:
https://bugzilla.mozilla.org/show_bug.cgi?id=1070710
Hopefully the new code does not require a version of OSX greater than 10.6 (Firefox 49 requires 10.9 or newer).

comment:6 Changed 3 years ago by arlolra

That is probably because Apple added an exception for Firefox <= 48

I see. Thanks for clarifying.

comment:7 Changed 3 years ago by gk

Cc: macsmerf added

Resolved #20233 as duplicate.

comment:8 Changed 3 years ago by gk

Keywords: TorBrowserTeam201609 added

comment:9 Changed 3 years ago by mcs

Cc: sukhbir added

Kathy and I backported the three Mozilla patches from https://bugzilla.mozilla.org/show_bug.cgi?id=1070710. They did not apply cleanly due to other changes Mozilla has made since ESR 45, but the changes we had to make were not extensive.

So far Kathy and I have only tested these patches with Tor Browser on OSX 10.11.6 (we do not yet have access to a 10.12/Sierra system; Kathy and I will upgrade one of our computers soon). I am posting the patches now because the Tor Messenger team would like to include a fix for this ticket in their next release, which is imminent.

Please take the most recent three bug20204-01 branch commits from brade's tor-browser repo. Via gitweb:

https://gitweb.torproject.org/user/brade/tor-browser.git/log/?h=bug20204-01

(all three commits have author Markus Stange).

After we have tested on a Sierra system I will mark this ticket as ready for review.

comment:10 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201609R added; TorBrowserTeam201609 removed
Status: newneeds_review

I upgraded a MacBook Pro to Sierra. These patches seem to fix the window drag problem.

Last edited 3 years ago by mcs (previous) (diff)

comment:11 Changed 3 years ago by gk

arlolra: how fast do you need that get reviewed? Or are you just cherry-picking the patches? Would it be enough to have it done on Monday?

comment:12 Changed 3 years ago by arlolra

gk: Monday is fine, or thereafter. We were hoping to release later in the week; still blocked on a few other things though. Thanks!

comment:13 Changed 3 years ago by arlolra

I blindly applied the patches in,
https://gitweb.torproject.org/tor-messenger-build.git/commit/?id=eb37fc7a210b86efd6a407b8527a41cb1330ebfb

And built,
https://paganini.erinn.org/~arlolra/tor-messenger/tor-messenger-0.2.0b2-osx-x86_64-59e68f.dmg

which seems to fix the issue for us as well. Thanks Kathy; thanks Mark.

Awaiting review to consider the matter resolved though.

comment:14 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good to me. I take that for the next alpha (commits 4dc2922a3071f1789cb0078809e4b28c48fb3922, 037b95c9574e274274441a17d3f304d529417f7d and 7298e8412e723ff90d74dd06124f8b1ae7e12980 on tor-browser-45.4.0esr-6.5-1) but am not sure about the stable yet. mcs: do you think we should just take those patches there, too, without further testing?

comment:15 in reply to:  14 ; Changed 3 years ago by mcs

Replying to gk:

mcs: do you think we should just take those patches there, too, without further testing?

Yes. This problem is annoying enough to users that I think we should take that risk.

comment:16 Changed 3 years ago by mcs

Resolved #20282 as duplicate.

comment:17 Changed 3 years ago by mcs

Cc: bitlox added

comment:18 in reply to:  15 Changed 3 years ago by gk

Replying to mcs:

Replying to gk:

mcs: do you think we should just take those patches there, too, without further testing?

Yes. This problem is annoying enough to users that I think we should take that risk.

Done with commits 95b008b232fb75f09541ac43e3d1ef5482ad2590, b3e5e45fc43646fa658cd8bbb43389a02e391150, and 8b1f07f31cd04fb157bf99a9e2e600b4422f277f on tor-browser-45.4.0esr-6.0-1.

comment:19 Changed 3 years ago by mcs

Repeating comments here that I made on IRC in case they were missed:

Kathy and I do not have as many different OSX versions as I hoped. Here is what we learned though:

  • on 10.6.8, dragging anywhere in the window (including on the scrollbar) moves the window.
  • on 10.11.6 and 10.12, the same problem occurs if you load a second page and then go back in history.

We have no way to test on 10.7 - 10.10 at the moment.
I cannot find a Mozilla bug for the 10.11.6 and 10.12 behavior, but I cannot reproduce the problem with Firefox 49 or 50 beta either. I will try ESR 45.4.0.

and a few minutes later:

  • I cannot reproduce the problem with Firefox ESR 45.4.0

comment:20 Changed 3 years ago by mcs

I found https://bugzilla.mozilla.org/show_bug.cgi?id=1070038 which describes similar symptoms, but it was fixed for Firefox 35. I guess the fact that ESR 45 does not show a problem is not that interesting because it does not contain the fixes we backported for this ticket (I keep getting confused by Apple's exception for Firefox <= 48).

comment:21 Changed 3 years ago by gk

Okay, I think the first thing we should do is making sure that really the backported patches are the culprit. I guess doing two builds, one with the latest tor browser 45.4.0esr code and one where just the backported patches are omitted and testing that on 10.11.6 could be a good idea.

If I did not mess up, hrm. I guess opening a new ticket to track our efforts could be helpful. Should we bother with asking Mozilla for help? Strictly speaking it is not their issue.

comment:22 in reply to:  21 Changed 3 years ago by mcs

Replying to gk:

Okay, I think the first thing we should do is making sure that really the backported patches are the culprit. I guess doing two builds, one with the latest tor browser 45.4.0esr code and one where just the backported patches are omitted and testing that on 10.11.6 could be a good idea.

Doing that would be conclusive, although since we do not see the problem with TB 6.5a3 it is almost certainly caused by the backported patches associated with this ticket.

If I did not mess up, hrm. I guess opening a new ticket to track our efforts could be helpful. Should we bother with asking Mozilla for help? Strictly speaking it is not their issue.

Probably someone just needs to debug it. Firefox 50 current beta does not show the problem. So maybe Kathy and I made a mistake in backporting the patches, or maybe there are other fixes in Firefox 50 that avoid the problem somehow.

comment:23 Changed 3 years ago by arlolra

For better or for worse, Tor Messenger is composed of several windows (#16493). Today I noticed that w/ these patches applied, the "contacts" window suffers from the dragging anywhere symptom (as described in comment:19). This doesn't seem to affect usability as much as not being able to move windows at all, so we may opt to release as is. We're also considering waiting for the next esr release in a couple weeks. Either way, no rush on our part.

comment:24 Changed 3 years ago by mcs

Resolution: fixed
Status: closedreopened

comment:25 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201610R added; TorBrowserTeam201609R removed
Status: reopenedneeds_review

It took us most of the day, but Kathy and I found the problem: when we modified one of the Firefox patches to avoid RectIter (which is not available in ESR 45), we introduced a bug that caused some non-draggable regions to be ignored: we were iterating twice each time through a loop. Please review the following "fixup" patch:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug20204-fixup-01&id=a6b4bb9a9d2769e8be110f2f0486b9ec74882575

comment:26 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Woah, that's subtle, good find. I tested a build with it and it fixes the issue for me. I applied the fixup patch to tor-browser-45.4.0esr-6.5-1 (commit a6b4bb9a9d2769e8be110f2f0486b9ec74882575).

comment:27 Changed 3 years ago by arlolra

Can this go on tor-browser-45.4.0esr-6.0-1 as well?

comment:28 in reply to:  27 Changed 3 years ago by gk

Replying to arlolra:

Can this go on tor-browser-45.4.0esr-6.0-1 as well?

It should. Done with commit 41f1c54ad978155f964fca1100f0c2eda1bef88a.

comment:29 Changed 3 years ago by arlolra

Thanks gk!

Also, thanks Kathy and Mark for hunting this down. Confirmed that fixes the issue in comment:23 as well.

comment:30 Changed 3 years ago by teor

There is a related bug I just found where clicking on the insertion point in some (multi-line?) editable text fields on macOS Sierra causes the window to be dragged. It's hard to reproduce, and not worth fixing, as it is so rare. But we should check that we don't make it worse with this fix.

comment:31 in reply to:  30 Changed 3 years ago by gk

Replying to teor:

There is a related bug I just found where clicking on the insertion point in some (multi-line?) editable text fields on macOS Sierra causes the window to be dragged. It's hard to reproduce, and not worth fixing, as it is so rare. But we should check that we don't make it worse with this fix.

Could you test that with

https://people.torproject.org/~gk/testbuilds/TorBrowser-tbb-nightly-osx64_20204_ALL.dmg
https://people.torproject.org/~gk/testbuilds/TorBrowser-tbb-nightly-osx64_20204_ALL.dmg.asc

or give us otherwise a hint on how we could check it?

comment:32 Changed 3 years ago by teor

I can't reliably reproduce it in TBB 6.0.5, so my best advice is to type some text in an editable multi-line field (like this one in Trac), and click and drag near the insertion point. Occasionally, the window moves, rather than the text being selected or moving.

comment:33 Changed 3 years ago by teor

Is that nightly build supposed to contain the control socket space fix from #20111?
Because it broke, and I had to set extensions.torlauncher.control_port_use_socket to false to get any Tor Browser to work, even 6.0.5. (They all share the same extensions, because the extensions are in Application Support, too.)

comment:34 Changed 3 years ago by teor

I couldn't reproduce the text field window dragging behaviour using the alpha, so it's no worse than 6.0.5. And the window dragging for the titlebar works. Thanks!

comment:35 Changed 3 years ago by gk

Replying to teor:

Is that nightly build supposed to contain the control socket space fix from #20111?
Because it broke, and I had to set extensions.torlauncher.control_port_use_socket to false to get any Tor Browser to work, even 6.0.5. (They all share the same extensions, because the extensions are in Application Support, too.)

*sigh*, sorry for that. I rebuilt the browser part lately quite often in my nightly setup to test various browser fixes. While reviewing and testing #20111 I copied the torbutton/tor-launcher .xpi over from a local build to avoid the overhead from our Gitian buils setup. To answer your question directly: no, the fix for #20111 is not in that bundle yet.

comment:36 Changed 3 years ago by teor

That's ok, and thanks, I'll test the control socket fix when it appears in the alphas.

Note: See TracTickets for help on using tickets.