1218797 - Toolbar-Button flickers on being pressed

Status: RESOLVED WORKSFORME

(Firefox :: Toolbars and Customization, defect)

Description

[Markus Jaritz [:designakt] (UX)]

Attachment: pressedButtonTest.gif

When changing the state of the hello button in the toolbar to pressed (to open the panel), the button outline changes from thin to thick, back to thin and back to thick again, before staying that way for the time it is pressed.
This behavior does not exist on the menu- and download-buttons.

Comment 1

[Mark Banner (:standard8)]

I think from your gif that this also applies to the whimsy button. Therefore I don't think this is hello specific, but more a general Firefox bug, so moving to a more appropriate product/component.

It could be something to do with `badged-button`s from a quick look in inspector.

Comment 2

[Markus Jaritz [:designakt] (UX)]

CCing Blake as he fixed it for Whimsy (and WebExtensions in general) today (bug 1217129).
This leaves Hello being the only button with the flickering, but he might know how to fix it.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

It started off not showing the pressed outline at all, so the problem with Whimsy/WebExtensions was one of my own making.  The fix for that was to move the:
    node.setAttribute("open", "true");
line up to https://reviewboard.mozilla.org/r/22945/diff/6#chunk2.3

I'm not sure where that attribute is being set in the Hello code, but I suspect it should be quite a bit earlier than it is.
(I tried looking for it for 10-20 minutes, but none of the things I found seemed to be called by the panel opening code.  Maybe the problem is that you're removing the attribute and then re-adding it?  I'm just guessing here, though…)

Comment 4

[Blake Winton (:bwinton) (:☕️)]

(Adding Standard8, cause it really is a Hello/Loop bug, not a general Firefox bug…  ;)

Comment 5

[Mark Banner (:standard8)]

Well here's a few relevant bits of code, but I don't see anything that is setting the open attribute. So I'm guessing this could be something in CustomizableWidget, but then I don't know why other panels aren't seeing it.

http://hg.mozilla.org/mozilla-central/annotate/cc48981c026c/browser/base/content/browser-loop.js#l74
http://hg.mozilla.org/mozilla-central/annotate/cc48981c026c/browser/components/customizableui/CustomizableWidgets.jsm#l955

cc'ing a couple of more folks who might have an idea.

Comment 6

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

(In reply to Blake Winton (:bwinton) (:☕️) from comment #3)
> It started off not showing the pressed outline at all, so the problem with
> Whimsy/WebExtensions was one of my own making.  The fix for that was to move
> the:
>     node.setAttribute("open", "true");
> line up to https://reviewboard.mozilla.org/r/22945/diff/6#chunk2.3
> 
> I'm not sure where that attribute is being set in the Hello code, but I
> suspect it should be quite a bit earlier than it is.
> (I tried looking for it for 10-20 minutes, but none of the things I found
> seemed to be called by the panel opening code.  Maybe the problem is that
> you're removing the attribute and then re-adding it?  I'm just guessing
> here, though…)

The attribute ought to be set and removed by the XUL popup code. I don't know offhand why it wasn't in Blake's testing, but JS shouldn't be messing with it manually. Fixing that for both hello (which does it here:

https://dxr.mozilla.org/mozilla-central/source/browser/modules/PanelFrame.jsm

) and the extension stuff should fix this issue, and will likely make the panels work better than they currently do anyway...

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Should the XUL popup code also be handling stuff like bug 1217129?

Comment 8

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

(In reply to Blake Winton (:bwinton) (:☕️) from comment #7)
> Should the XUL popup code also be handling stuff like bug 1217129?

Yes. That's quite possibly another consequence of manually messing with this attribute.

Comment 9

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

Hello went away, so I don't think this is an issue anymore.