1901551 - Fix vertical tabs breaking customize mode's "restore defaults" button

Status: RESOLVED FIXED

(Firefox :: Sidebar, task)

Description

[Sarah Clements [:sclements]]

Gijs found this during review of bug 1893655 when using customize mode with sidebar.verticalTabs enabled and pressing "restore defaults", the UI breaks and there's this error.

console.error: CustomizeMode: 
  Message: TypeError: aElement.hasAttribute is not a function
  Stack:
    getCustomizationTarget@resource:///modules/CustomizableUI.sys.mjs:839:16
findWidgetInWindow@resource:///modules/CustomizableUI.sys.mjs:1799:16
getWidgetNode@resource:///modules/CustomizableUI.sys.mjs:1359:21
buildArea@resource:///modules/CustomizableUI.sys.mjs:1153:37
_rebuildRegisteredAreas@resource:///modules/CustomizableUI.sys.mjs:3335:14
reset@resource:///modules/CustomizableUI.sys.mjs:3252:10
reset@resource:///modules/CustomizableUI.sys.mjs:4444:28
reset/<@resource:///modules/CustomizeMode.sys.mjs:1234:22

What's happening is that aElement happens to be the vertical-tabs slot we use to put tabbrowser-tabs in and its not a customization target. We don't want to make it a customization target, and part of this may be stemming from tabbrowser-tabs not being removable. We probably want to make it removable but perhaps only when the verticalTabs pref is true. This bug might be impacted by work done in bug 1899346, so I'll mark it as blocked by it for now.

Comment 1

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

So, if I understand this correctly, we've moved the #tabbrowser-tabs element to the sidebar when in vertical tabs. When we attempt to restore default, and loop over the customizable areas to find widgets that need moving/removing, the TabsToolbar area has tabbrowser-tabs as its 2nd placement (after the firefoxview button.) In its new position, #tabbrowser-tabs has no ancestor element which is a customization target, so we end up at the document node, which of course has no hasAttribute method.

In the discussion on the patch for Bug 1899346, :Gijs suggests that the simplest way forward for now might be to have the restore defaults button in the customization panel first reset back to horizontal tabs. That seems like a good first step here. It doesnt change the fact that any CUI method that calls getCustomizationTarget("tabbrowser-tabs") will end up throwing the same exception. That's a bit of a chicken and egg problem with Bug 1899346. I'm not sure if it will turn out to need to be fixed here first or there.

Comment 2

[Sarah Clements [:sclements]]

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

So, if I understand this correctly, we've moved the #tabbrowser-tabs element to the sidebar when in vertical tabs. When we attempt to restore default, and loop over the customizable areas to find widgets that need moving/removing, the TabsToolbar area has tabbrowser-tabs as its 2nd placement (after the firefoxview button.) In its new position, #tabbrowser-tabs has no ancestor element which is a customization target, so we end up at the document node, which of course has no hasAttribute method.

Yeah, the CustomizeableUI code doesn't expect #tabbrowser-tabs to have moved outside of the TabsToolbar-customization-target element.

Comment 3

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

Possibly I'm missing something obvious, but how does making it removable help unless we also make the sidebar a widget area? We're still not be able find a customization target for it, and isWidgetRemovable throws when it tries to find the node again. I guess I've not really understood all the CUI logic yet - I'm just trying to figure out if there was a plan here or if the removable thing was just a potential direction to explore.

Comment 4

[Sarah Clements [:sclements]]

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

Possibly I'm missing something obvious, but how does making it removable help unless we also make the sidebar a widget area? We're still not be able find a customization target for it, and isWidgetRemovable throws when it tries to find the node again. I guess I've not really understood all the CUI logic yet - I'm just trying to figure out if there was a plan here or if the removable thing was just a potential direction to explore.

This was definitely not a "we have to make it removeable", but rather a possible direction to explore that Gijs mentioned during review of the initial patch.

Comment 5

[:Gijs (he/him)]

(In reply to Sarah Clements [:sclements] from comment #4)

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

Possibly I'm missing something obvious, but how does making it removable help unless we also make the sidebar a widget area? We're still not be able find a customization target for it, and isWidgetRemovable throws when it tries to find the node again. I guess I've not really understood all the CUI logic yet - I'm just trying to figure out if there was a plan here or if the removable thing was just a potential direction to explore.

This was definitely not a "we have to make it removeable", but rather a possible direction to explore that Gijs mentioned during review of the initial patch.

Yeah - I had not even gotten as far as actually investigating the stack in detail (like comment 1). Here is the entire para that led to this bug being filed:

Yet another option would be to actually make tabbrowser-tabs removable, and then fix up whatever else is complaining in customizableUI. It likely still couldn't actually be put anywhere else by users easily because in customize mode we don't add "normal" customize mode drag/drop behaviour to the tabstrip, so you can still switch tabs - though I'm sure people would figure out ways around that using the browser console or by manipulating prefs or w/e, I expect people already do that anyway so it's probably no huge deal. This is probably the easiest option - assuming it actually helps with the reset functionality.

(Emphasis added. :-) )

So mostly the issue is that "restore defaults" is completely broken once this pref is turned on. We should fix that.

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

In the discussion on the patch for Bug 1899346, :Gijs suggests that the simplest way forward for now might be to have the restore defaults button in the customization panel first reset back to horizontal tabs. That seems like a good first step here. It doesnt change the fact that any CUI method that calls getCustomizationTarget("tabbrowser-tabs") will end up throwing the same exception. That's a bit of a chicken and egg problem with Bug 1899346. I'm not sure if it will turn out to need to be fixed here first or there.

FWIW, in that patch I was actually asking not about the tabbrowser-tabs themselves as much as I was about the other widgets, as the alltabs-button is not marked as removable (and I thought the same would be true for the new tab button, but that's already removable). Whether those widgets are removable will affect how happy CUI is with that patch moving them to other places (other toolbars or just the palette, if we don't think they should be visible in the tabstrip). When I commented, your patch left those widgets physically in the tabstrip, and I was trying to suggest that instead we could just removeWidgetFromArea those widgets if we made them removable.

The upshot would be that we could copy the tabstrips pre-vertical-tabs placement list to a separate pref, and for any of the default widgets we could call removeWidgetFromArea, and any others we could put in the toolbar. Then if/when the user turns off vertical tabs, we can rebuild the area with the original set of placements. That seems neater than than making the widgets permanently unavailable (esp. the new tab button, but even the all tabs button could be something users want, if their tabs do not fit in the sidebar and the scrolling is annoying - much like it can be with horizontal tabs.)

Comment 6

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

Attachment: WIP: Bug 1901551 - Handle the vertical-tabs case when finding widgets in CUI. r?#tabbrowser-reviewers!

Comment 7

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

Attachment: Bug 1901551 - Make the vertical-tabs element customizable=true r?#sidebar-reviewers

Comment 8

[William Durand [:willdurand]]

FWIW I submitted an exploratory patch on Try to see which extension tests would fail when we enable vertical tabs. It looks like many tests are failing on the same error, which looks a lot like the one in Comment 0.

Comment 9

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

The patch on this bug should address at least some of those errors. Anything that calls into or triggers customizable UI functions will likely end up calling getWidgetNode and run into this same issue: we assume a widget has some ancestor element that is a customizable area, and the loop ends up calling .hasAttribute on the document node. The concerns in comment #0 are really about user-facing customization options in the customize toolbar UI, not whether something is "customizable" i.e. findable by these functions.

That said, we might want to add an alternate manifest which runs some relevant subset of the extensions test with the sidebar.revamp and sidebar.verticalTabs presf set to true to avoid last-minute surprises.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2825def1df7b
Make the vertical-tabs element customizable=true r=mconley

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/2825def1df7b