1251902 - "Hello" and "Share this page" buttons sometimes blink when I click them to open panel

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P3)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 47, 32bit, ID 20160225030209
STR:
1. Move "Share this page" and "Hello" buttons to navigation toolbar
2. Open page https://www.mozilla.org/en-US/ (somehow heavy for "share this page")
3. Click "Share this page" / "Hello"  toolbarbutton

AR:
 The toolbarbutton blinks white-black-white-black:
 First it's white, because mouse pointer is placed above
 -> then it's black, because it's pressed
 -> then it's white, because I released left mouse button, but it's still above the button
 -> then it's black, because widget panel is open

ER:  
 Just like all other built-in buttons: only "white->black" change of color when I click button

Notes:
 This doesn't happen with add-ons: uBlock, Tab Data, Simplified Tab Groups. I haven't looked precisely
 on how those 2 toolbarbuttons are implemented, but probably this could be helped even on "Theme" side
 I hope that somebody will assign the right component.

Screencast (you can see both types of behavior - AR for Hello and ER for others)
> ‎(2015.10.29 13-55-16)   https://dl.dropboxusercontent.com/s/i7s55z1gle0n19r/screencast%201%20-%20Widget%20icons%20sometimes%20blink.webm?dl=0

Comment 1

[magicp]

(In reply to arni2033 from comment #0)

> Just like all other built-in buttons: only "white->black" change of color when I click button

Interesting. This behavior is easy to see in bookmarks toolbar. (button-down > button-up > button-down)
In step 1, Move "Share this page" and "Hello" buttons to bookmarks toolbar

Comment 2

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

I expect the issue is that the 'open' attribute gets set much later for these buttons, or something along those lines - it seems to work fine on most other buttons, so moving this to loop for them to diagnose. Once we have a diagnosis it should be possible to apply that to the share page button; either way it doesn't seem plausible there's a theme issue considering it works with the other buttons.

Comment 3

[Mark Banner (:standard8)]

Attachment: Stop the 'Hello' and 'Share this page' buttons blinking when the panel is being opened.

This is a prototype I've whipped up - the issue is in the social code. I think as Gijs said, we're just setting the open attribute way too late.

This should fix it, though I'm doing a bit more testing before getting review.

Comment 4

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

FWIW, I've never understood why this code needs to set the attribute "manually", as it were - the attribute is normally set (and removed) on its anchor by the panel, I believe.

Comment 5

[Mark Banner (:standard8)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e451e926a2c

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8725631 [details] [diff] [review]
Stop the 'Hello' and 'Share this page' buttons blinking when the panel is being opened.

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

I'm not sure why there's no automatic updating here. Maybe Shane knows?

I've tried this a few more times and it seems fine to fix the bug at least (and would probably be good for uplift). Obviously, if there's a way to get automatic, we could deal with that in a follow-up.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8725631 [details] [diff] [review]
Stop the 'Hello' and 'Share this page' buttons blinking when the panel is being opened.

tbh I don't recall why it's necessary to set the attribute.  I'd have to spend some time digging into that issue.  I think this is a fine fix for now.

Comment 8

[Mark Banner (:standard8)]

Attachment: Stop the 'Hello' and 'Share this page' buttons blinking when the panel is being opened.

Shane: Try server revealed that the patch broke the browser_UITour_loop_panel test. This adjusts the test to on the panel state to be open rather than the button state, since we're adjusting where these opens are set.

Comment 9

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/d3b650bd44960ba00e28a0a6b539828c0179ccf9
Bug 1251902 - Stop the 'Hello' and 'Share this page' buttons blinking when the panel is being opened. r=mixedpuppy

Comment 10

[Carsten Book [:Tomcat]]

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