1899346 - Move all tools away from the tabs toolbar when in vertical tabs mode

Status: RESOLVED FIXED

(Firefox :: Sidebar, task, P1)

Description

[Sarah Clements [:sclements]]

Per the spec when a user changes from horizontal tabs to vertical tabs we want to hide the tabs-toolbar and all tools in there - default or custom pinned - should go in the nav toolbar and/or overflow menu and back into their default position when toggled back to horizontal tabs.

This is the area of CustomizableUI code we'll want to address.

Moving the red/yellow/green window controls into the nav bar will be handled in a separate bug.

Comment 1

[Sarah Clements [:sclements]]

Something to flag that came up during review of bug 1893655, is that trying to restore defaults when in customize mode breaks the UI. I've filed a separate bug for handling the tabstrip specifically (in terms of making it removable, see bug 1901551). But we'll also need to think about this when moving these widgets into navbar/overflow that restoring defaults doesn't try to immediately place them out of the navbar and back into the tabs-toolbar area (perhaps it will just behing the scenes update the defaultPlacements).

Comment 2

[Sam Foster [:sfoster] (he/him)]

Attachment: WIP: Bug 1899346 - Move all tools in the tabs toolbar when in vertical tabs mode. r?#sidebar-reviewers!

Comment 3

[Sam Foster [:sfoster] (he/him)]

I have a exploratory/WIP patch on here with a number of open questions that I'm not sure how to answer and proceed with the patch. Need-infoing sclements and :Gijs, please forward the need-info as appropriate.

Comment 4

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] (he/him) from comment #3)

I have a exploratory/WIP patch on here with a number of open questions that I'm not sure how to answer and proceed with the patch. Need-infoing sclements and :Gijs, please forward the need-info as appropriate.

I've added comments on phab.

Comment 5

[Sarah Clements [:sclements]]

I've also added comments on phab.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1899346 - Move CUI widgets out of the horizontal tabstrip when in vertical tabs mode. r?#sidebar-reviewers!,mconley!

Depends on D215986

Comment 7

[Sam Foster [:sfoster] (he/him)]

I had a couple of reviewers suggest that the "reset" button in the customize toolbars panel should also toggle vertical tabs back to horizontal. Throughout the customizable UI component there is this notion of the "default" state and having default-but-vertical-tabs was making things extra complicated. So for this bug/patch, I've implemented this suggestion, meaning enabling vertical tabs puts the UI in a non-default state which can be reset.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c7c121f1fa3
Move CUI widgets out of the horizontal tabstrip when in vertical tabs mode. r=sidebar-reviewers,mconley,sclements,webdriver-reviewers,whimboo

Comment 9

[Cristina Horotan [:chorotan]]

Backed out changeset 0c7c121f1fa3 (bug 1899346) for causing Mn failures at test_quit_restart.py

Backout: https://hg.mozilla.org/integration/autoland/rev/f7f1262ac5e5a5a17df34104442ac3e0df2e60a5

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=0c7c121f1fa3d38d88c7a049a2f477369ac03432&selectedTaskRun=XQDe358CSW6uZJNujTEV4Q.0

Failure logs:
https://treeherder.mozilla.org/logviewer?job_id=472802317&repo=autoland&lineNumber=15187
https://treeherder.mozilla.org/logviewer?job_id=472804794&repo=autoland&lineNumber=161497

Comment 10

[Sam Foster [:sfoster] (he/him)]

Any idea what is going on here :whimboo? Unless I did something wrong with how I registered the new marionette test suite, I can't see how my patch could cause a failure in the testing/marionette/harness/marionette_harness/tests/unit tests? What am I missing here?

Comment 11

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

The problem here seems to be that the following JavaScript error, which can be seen in the logs, causes breakage in the startup phase causing Firefox not correctly to initialize.

JavaScript error: chrome://browser/content/sidebar/browser-sidebar.js, line 1532: TypeError: can't access property "getAttribute", arrowScrollbox is null

Because the above error isn't caught all the remaining startup code isn't run, and as result no browser-idle-startup-tasks-finished notification is sent out, which is required to initialize Marionette.

Comment 12

[Sam Foster [:sfoster] (he/him)]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #11)

Because the above error isn't caught all the remaining startup code isn't run, and as result no browser-idle-startup-tasks-finished notification is sent out, which is required to initialize Marionette.

Ah ha! Thank you - I had not spotted that. That makes sense and is something I'd seen before (and thought I'd fixed). I've not been able to get this suite to run locally to reproduce. Do you happen to know what the trick is? I get a AttributeError(f"module {__name__!r} has no attribute {name!r} exception when I mach test testing/marionette/harness/marionette_harness/tests/unit/unit-tests.toml

Comment 13

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

Oh there might be a bug in mach test. Mind filing a bug under the Marionette client and harness component? Otherwise to run the tests you can use mach marionette-test instead. I would suggest to use the -vv --gecko-log - options as well which will print tracing details and all logs from Gecko to the console. It's very helpful for investigating issues.

Comment 14

[Sam Foster [:sfoster] (he/him)]

More marionette questions, as I'm still unable to run this test locally on my mac. I'm on recent moz-central and I've done a ./mach bootstrap.
I'm trying: ./mach marionette-test -vv testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py and the runner starts up, opens the first browser window, then it stops with the following:

AttributeError: module 'unittest' has no attribute '_TextTestResult'. Did you mean: 'TextTestResult'?

  File "/Users/sfoster/mozilla-unified/testing/marionette/mach_commands.py", line 113, in marionette_test
    return run_marionette(tests, topsrcdir=command_context.topsrcdir, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sfoster/mozilla-unified/testing/marionette/mach_commands.py", line 58, in run_marionette
    failed = MarionetteHarness(MarionetteTestRunner, args=vars(args)).run()
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sfoster/mozilla-unified/testing/marionette/harness/marionette_harness/runtests.py", line 79, in run
    runner.run_tests(tests)
  File "/Users/sfoster/mozilla-unified/testing/marionette/harness/marionette_harness/runner/base.py", line 1016, in run_tests
    self.run_test_sets()
  File "/Users/sfoster/mozilla-unified/testing/marionette/harness/marionette_harness/runner/base.py", line 1242, in run_test_sets
    self.run_test_set(self.tests)
  File "/Users/sfoster/mozilla-unified/testing/marionette/harness/marionette_harness/runner/base.py", line 1212, in run_test_set
    self.run_test(test["filepath"], test["expected"])
  File "/Users/sfoster/mozilla-unified/testing/marionette/harness/marionette_harness/runner/base.py", line 1176, in run_test
    results = runner.run(suite)
              ^^^^^^^^^^^^^^^^^
  File "/Users/sfoster/mozilla-unified/testing/marionette/harness/marionette_harness/runner/base.py", line 254, in run
    result = super(MarionetteTextTestRunner, self).run(test)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sfoster/mozilla-unified/testing/mozbase/moztest/moztest/adapters/unit.py", line 207, in run
    test(result)
  File "/opt/homebrew/Cellar/python@3.12/3.12.2_1/Frameworks/Python.framework/Versions/3.12/lib/python3.12/unittest/suite.py", line 84, in __call__
    return self.run(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.12/3.12.2_1/Frameworks/Python.framework/Versions/3.12/lib/python3.12/unittest/suite.py", line 122, in run
    test(result)
  File "/opt/homebrew/Cellar/python@3.12/3.12.2_1/Frameworks/Python.framework/Versions/3.12/lib/python3.12/unittest/case.py", line 690, in __call__
    return self.run(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sfoster/mozilla-unified/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 247, in run
    result.stopTest(self)
  File "/Users/sfoster/mozilla-unified/testing/marionette/harness/marionette_harness/runner/base.py", line 225, in stopTest
    unittest._TextTestResult.stopTest(self, *args, **kwargs)
    ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.12/3.12.2_1/Frameworks/Python.framework/Versions/3.12/lib/python3.12/unittest/__init__.py", line 85, in __getattr__
    raise AttributeError(f"module {__name__!r} has no attribute {name!r}")

python3 --version gives me: Python 3.12.2. It looks like a python or library issue. Any suggestions? Right now I'm having to push to try to investigate and it is slow going.

Comment 15

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

Sam, can you please try with Python 3.11? It could indeed be an issue with Python 3.12. So far we haven't noticed any issue with 3.11. Maybe please file a bug in Testing : Marionette Client and Harness so that we can get it addressed.

Comment 16

[Sam Foster [:sfoster] (he/him)]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #15)

Sam, can you please try with Python 3.11? It could indeed be an issue with Python 3.12. So far we haven't noticed any issue with 3.11. Maybe please file a bug in Testing : Marionette Client and Harness so that we can get it addressed.

I filed bug 1917379 with the 3.12 exception and also the results from 3.11 - which ran the test to completion but fell over at the end by the looks of it.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b50071da8b63
Move CUI widgets out of the horizontal tabstrip when in vertical tabs mode. r=sidebar-reviewers,mconley,sclements,webdriver-reviewers,whimboo,tabbrowser-reviewers

Comment 18

[Atila Butkovits]

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