1753861 - On macOS, setting the home page by dragging tabs breaks the tabs.

Status: VERIFIED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect, P2)

Description

[coeurl]

Attachment: fx.gif

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:96.0) Gecko/20100101 Firefox/96.0

Steps to reproduce:

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:96.0) Gecko/20100101 Firefox/96.0

On macOS Firefox 96. Other versions may also occur.

1. Open multiple tabs
2. Drag one of the tabs and drop it on the home icon
3. You will be asked if you want to set it as your home page, so answer either way
4. This will interrupt the dragging process and place the tabs in a strange position.

Actual results:

The tab had fixed in a strange position.

Expected results:

Tab movement is completed, or the tab returns to the position before the movement.

Comment 1

[coeurl]

I am reporter. I have a question about using Bugzilla, where can I edit the body of a report? I couldn't find an editable form.

Comment 2

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

The Bugbug bot thinks this bug should belong to the 'Core::Widget: Cocoa' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 3

[Stephen A Pohl [:spohl]]

mozregression points to https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9cf59af57183940e3d5f629f4ea29b499e956fca&tochange=1129347e2aadcde6d60e03b6917539413c306639.

Note, on newer versions of Firefox, the "Home" icon has to be added back to the Toolbar by customizing it.

Gijs, could you take a look?

Comment 4

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

Set release status flags based on info from the regressing bug 1685313

Comment 5

[:Gijs (out for now; he/him)]

Attachment: Bug 1753861 - fix dragging to home button breaking the tabstrip by ending drag sessions for internally opened dialogs too, r?smaug

This also fixes bug 1721308.

Comment 6

[:Gijs (out for now; he/him)]

Attachment: Bug 1753861 - end drag sessions before opening any subdialogs, r?mconley

When opening new 'real' windows, the window mediator/watcher code takes care of this.
For windows inside frames, such as the SubDialog code uses, it does not.
The reason this is needed is related to a few different things:

• drag processing is different on different OSes. For example, on Windows,
  once we're getting a 'drop' event we'll get a 'dragend' (which sorts out
  the tabstrip dragging) immediately afterwards. On macOS, this goes via
  the event loop. So some of the timing of this is unpredictable.
• dialogs can call 'sizeToContent', which in turn checks if the window
  can even be resized by the calling principal, which suspends drag
  operations (see bug 329385), which ends the drag without notifying
  dragend, which in turn makes things sad.
• dialog opening code such as the prompter code spins event loops. This
  messes with event handling some more.

As SubDialog is the common bit between all 3 types of prompts
(browser window, tab, content modal) that don't open separate 'real'
external windows, this seems the logical place to put this.

Unfortunately no tests because simulating native drag events is...
not straightforward.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Does the patch fix also bug 1544167, I mean, could we back the patch there out?

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Olli Pettay [:smaug] from comment #7)

Does the patch fix also bug 1544167, I mean, could we back the patch there out?

No; the issues there are/were around opening "real" (ie OS-level) windows and they still need some kind of fix. See also bug 1751866 comment 10 suggesting that we will also need a fix for OS-native dialogs like filepickers... The new patch I put up on this bug only cares about the strange-not-quite-real-windows that SubDialog.jsm creates for in-window prompts/dialogs.

However, I have been thinking about this situation some more and I'm starting to think (based on e.g. reading bug 1197598 and bug 948082 and bug 1001549) that maybe instead of adding EndDragSession(true) calls we should be more cautious about calling EndDragSession(false) and/or suppressing drags when windows move/resize? Fundamentally, when chrome code calls sizeToContent we shouldn't need to disrupt drags in the way that was needed in bug 329385. Olli, wdyt?

(the flip side there is that if it is impossible to drop things because we stop event interaction with the window, cancelling the drag is probably the right thing to do - but this isn't the case for the in-content tab-modal prompts that this bug cares about, nor for desktop notifications)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b42b18204f20
end drag sessions before opening any subdialogs, r=mconley

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(clearing the leftover needinfo)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Oh, there was a real needinfo.
Yeah, the EndDragSession in the window resize code could be probably removed when used in the parent process.

Comment 12

[Cristian Tuns]

Backed out for causing Pup failures with dialog.spec.ts

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-TIMEOUT | Page.Events.Dialog should fire (dialog.spec.ts) | expected PASS

Comment 13

[:Gijs (out for now; he/him)]

(In reply to Cristian Tuns from comment #12)

Backed out for causing Pup failures with dialog.spec.ts

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-TIMEOUT | Page.Events.Dialog should fire (dialog.spec.ts) | expected PASS

How can I run this test? I looked at https://firefox-source-docs.mozilla.org/remote/Testing.html which only has instructions to run all the tests. That doesn't work:

['npm', 'install']
Error running mach:

    ['puppeteer-test']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.
You can invoke |./mach busted| to check if this issue is already on file. If it
isn't, please use |./mach busted file puppeteer-test| to report it. If |./mach busted| is
misbehaving, you can also inspect the dependencies of bug 1543241.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

RuntimeError: Process hasn't been started yet

  File "remote\mach_commands.py", line 660, in puppeteer_test
    install_puppeteer(command_context, product, ci)
  File "remote\mach_commands.py", line 704, in install_puppeteer
    npm(command, cwd=os.path.join(command_context.topsrcdir, puppeteer_dir), env=env)
  File "remote\mach_commands.py", line 214, in npm
    wait_proc(p, cmd="npm", exit_on_fail=kwargs.get("exit_on_fail", True))
  File "remote\mach_commands.py", line 227, in wait_proc
    p.kill()
  File "testing\mozbase\mozprocess\mozprocess\processhandler.py", line 973, in kill
    raise RuntimeError("Process hasn't been started yet")

Sentry event ID: b05a925c50d14e5dbc94de573a28ab77
Sentry is attempting to send 0 pending error messages

Then I tried to figure out how to run just one test, which the fx-source-docs docs say you do by reading the puppeteer docs, but they don't seem to have any information about that - they just suggest to use --subset when you somehow (they don't say how!) have configured it to run fewer tests. They do say how to run only one task per test file (modifying the test file), but not how to run a specific test file. I tried passing just the path as an argument but that produced the same failure output quoted above.

I also tried to use mach test but that doesn't seem to understand puppeteer tests, which is unfortunate.

Comment 14

[:Gijs (out for now; he/him)]

Looks like nobody has ever run this on Windows or without node/npm in their $PATH? OK... I filed bug 1759248, but I still need an answer to how to run just 1 test.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to :Gijs (he/him) from comment #13)

Then I tried to figure out how to run just one test, which the fx-source-docs docs say you do by reading the puppeteer docs, but they don't seem to have any information about that - they just suggest to use --subset when you somehow (they don't say how!) have configured it to run fewer tests. They do say how to run only one task per test file (modifying the test file), but not how to run a specific test file. I tried passing just the path as an argument but that produced the same failure output quoted above.

Thanks for pointing that out. I've filed bug 1759434 to get the necessary information to our docs added. In short use the .only suffix to tests/groups to exclusively run these.

Comment 16

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

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Gijs, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 17

[:Gijs (out for now; he/him)]

The patch bounced because of failures in a test I couldn't run, and I haven't had a chance to look at this again since fixing the tests to at least run on my machine.

Comment 18

[Pascal Chevrel:pascalc]

Gijs, is that still something you are working on? Thanks

Comment 19

[:Gijs (out for now; he/him)]

Yes, but too much higher prio stuff going on so haven't been able to get back to it so far. It's pretty close to the top of my pile so hopefully this week or next.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/afbcc32c88d2
end drag sessions before opening any subdialogs, r=mconley

Comment 21

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/afbcc32c88d2

Comment 22

[Rares Doghi, Desktop QA]

Verified as fixed in our latest Beta build 110.0b5.