906634 - buttons in navbar seems to have "customizableui-anchorid" attribute set even if they're not in a panel

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Matteo Ferretti [:zer0] [:matteo]]

As explained here https://bugzilla.mozilla.org/show_bug.cgi?id=880458#c7, when a SDK Widget is added to the nav-bar, the attribute "customizableui-anchorid" is present, and set to "nav-bar-overflow-button" even if the widget itself is not "overflowed", and there is no overflow menu / chevron displayed.

The SDK Widget is added in this way, from add-on sdk environment:

    require('sdk/widget').Widget({
      id: 'foo',
      label: 'foo',
      content: 'X'
    });

Notice however, than it's not related to SDK widget only. It seems that all the button in the navbar, when I start the UX build with a fresh profile, have all the attribute "customizableui-anchorid" set to "nav-bar-overflow-button":

    [12:57:52.559] "urlbar-container" "nav-bar-overflow-button"
    [12:57:52.559] "search-container" "nav-bar-overflow-button"
    [12:57:52.560] "webrtc-status-button" "nav-bar-overflow-button"
    [12:57:52.560] "bookmarks-menu-button" "nav-bar-overflow-button"
    [12:57:52.560] "downloads-button" "nav-bar-overflow-button"
    [12:57:52.560] "home-button" "nav-bar-overflow-button"
    [12:57:52.561] "social-share-button" "nav-bar-overflow-button"
    [12:57:52.561] "social-toolbar-item" "nav-bar-overflow-button"

This log is generated running in Scratchpad the following code:

    let nodes document.querySelectorAll('[customizableui-anchorid]');  

    Array.forEach(nodes, node => console.log(node.id, node.getAttribute('customizableui-anchorid')));

I think is a bug because, when the buttons are moved and we're not in a fresh profile, they seems not having any "customizableui-anchorid". And it seems reasonable to do not have this attribute, if the buttons are not in any panel – so there is no "anchor" to refer – but at least, I would assume the value won't be pointing at "nav-bar-overflow-button".

Comment 1

[:Gijs (he/him)]

Mike, when reviewing bug 899576, I thought this was just doing what we were always doing - we were just doing it in buildArea before. Is that true, and if so, any clue why were we always doing this?

Should we only set this when the nodes are in the panel? I recall this situation being slightly schizophrenic in that the overflowable toolbar code *also* sets this. In fact:

http://mxr.mozilla.org/projects-central/source/ux/browser/components/customizableui/src/CustomizableUI.jsm?mark=2324,2352#2312

seems to do the Right Thing here. And this:

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #1)
> I think is a bug because, when the buttons are moved and we're not in a
> fresh profile, they seems not having any "customizableui-anchorid".

Would be a bug if they're all supposed to have this attribute... can you clarify what our desired end-state is? I think it makes sense not to have an anchor when the buttons are not overflowed, but I'd like to be sure, as ISTR we discussed this on IRC and I was missing some point of it. :-)

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I think it's reasonable to only have the attribute on the node when it's in either the overflow panel or menu panel.

Not sure why we applied it to all toolbar nodes with an empty string value - that sounds pretty misguided. :)

Comment 3

[:Gijs (he/him)]

Attachment: don't use anchor/anchorid for buttons that are in toolbars but not overflown,

I think this should work. I don't think anything other than the SDK currently really uses this (well, that and the downloads panel after bug 881905). But please check that I'm sane in checking where this is used...

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 802391 [details] [diff] [review]
don't use anchor/anchorid for buttons that are in toolbars but not overflown,

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

LGTM!

Comment 5

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/339479a60c1c

Comment 6

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/339479a60c1c