Vasilica Tamas, QA [:vtamas] Reporter
	Description • 8 years ago

Reproducible on: Firefox 46.0a1 and Firefox 45.0a2 across all platforms

STR
1.Launch Firefox with a clean profile.
2.Create a Firefox account and confirm it.
3.Log in using the recently created account.
4.Open the Panel Menu -> Click on 'Synced tabs' icon.
5.Click on “Firefox for Android” or “Firefox for iOS” link.

ER
Once the “Firefox for Android” or “Firefox for iOS” page is opened the Synced Tabs panel disappears.

AR
The Synced Tabs panel remains displayed while the “Firefox for Android” or “Firefox for iOS” page is opened.
See screenshot: http://i.imgur.com/jjcuLX4.jpg


Additional notes:
- Reproducible on: Firefox 46.0a1 (2016-01-07) and Firefox 45.0a2 (2016-01-07) using Windows 10 64-bit, Mac 10.10.5 and Ubuntu 14.04 32-bit.

	Mark Hammond [:markh] [:mhammond] Assignee
	Comment 1 • 8 years ago

The issue here is that the text-link widget is responsible for opening the link, but it obviously doesn't know anything about the panel. There's also bug 1237942, which reports that the links can't be clicked using other mouse buttons - and while that bug mentions the test-link widget probably can be changed to support other buttons, that still leaves us with the fact the panel would remain open and no obvious way to intercept or detect that.

So this patch attempts to kill both birds with one stone:
* An explicit click handler that handles all buttons and explicitly opens the link in a tab (ie, thereby fixing bug 1237942)
* That handler also closes the panel UI (ie, fixing this bug)

I've also attempted to handle a RETURN key opening the link, but given the panel UI isn't accessible it's tricky to manually test this aspect - so I'm not sure if it is worthwhile. However, I think I can add tests for both clicks and RETURN - I just haven't done that until I get feedback on the general approach.

	:Gijs (he/him)
	Comment 2 • 8 years ago

Comment on attachment 8709749 [details] [diff] [review]
0002-Bug-1237945-ensure-panel-is-closed-when-mobile-promo.patch

Sadly, I wouldn't worry too much about the keyboard case right now... we really need to sort out the keyboard nav in a comprehensive fashion at some point. Right now there isn't really a lot we can do about it, and the keyboard event handler doesn't really help because there's no reasonable way you can focus the links anyway.

Otherwise, this looks OK to me generally.

	Mark Hammond [:markh] [:mhammond] Assignee
	Comment 3 • 8 years ago

Thanks for the typically prompt feedback!

(In reply to :Gijs Kruitbosch from comment #2)
> Sadly, I wouldn't worry too much about the keyboard case right now...

Fair enough - I've made this change, and also another small change so the final URL is built in the click handler rather than directly in onCreated - this way the test can tweak the preference after the widget is built and have the new pref value used.

	:Gijs (he/him)
	Comment 4 • 8 years ago

Comment on attachment 8710268 [details] [diff] [review]
0002-Bug-1237945-ensure-panel-is-closed-when-mobile-promo.patch

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

::: browser/components/customizableui/test/browser_967000_button_sync.js
@@ +36,5 @@
> +  event.initMouseEvent("click", true, true, window,
> +                       0, 0, 0, 0, 0,
> +                       ctrlKeyArg, altKeyArg, shiftKeyArg, metaKeyArg,
> +                       buttonArg, null);
> +  target.dispatchEvent(event);

Why can't we use EventUtils?

@@ +169,5 @@
> +  for (let link of links) {
> +    for (let button = 0; button < 3; button++) {
> +      simulateClick({ button }, link);
> +      // the panel should have been closed.
> +      ok(!isPanelUIOpen(), "click closed the panel");

I don't think a right-click here should close the panel, or open the links...

	Mark Hammond [:markh] [:mhammond] Assignee
	Comment 5 • 8 years ago

(In reply to :Gijs Kruitbosch from comment #4)
> Why can't we use EventUtils?

We can - I didn't think to look there :(

> I don't think a right-click here should close the panel, or open the links...

Fair enough - fix this too - right-click is now ignored.

Thanks!

	:Gijs (he/him)
	Comment 6 • 8 years ago

Comment on attachment 8710864 [details] [diff] [review]
0002-Bug-1237945-ensure-panel-is-closed-when-mobile-promo.patch

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

LGTM, thanks!

PS: can you find/file a followup for the middle-click-on-text-link thing? That seems very fixable...

	Pulsebot
	Comment 7 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/2cb984c700cb

	Phil Ringnalda (:philor)
	Comment 8 • 8 years ago

Backed out in https://hg.mozilla.org/integration/fx-team/rev/4a57702113ed for frequent Linux32 debug e10s and ASan e10s https://treeherder.mozilla.org/logviewer.html#?job_id=6790815&repo=fx-team and https://treeherder.mozilla.org/logviewer.html#?job_id=6790814&repo=fx-team

	Mark Hammond [:markh] [:mhammond] Assignee
	Comment 9 • 8 years ago

(In reply to :Gijs Kruitbosch from comment #6)
> PS: can you find/file a followup for the middle-click-on-text-link thing?
> That seems very fixable...

Done - bug 1246082.

(In reply to Phil Ringnalda (:philor) from comment #8)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/4a57702113ed
> for frequent Linux32 debug e10s and ASan e10s

Lots of try runs later, it seems this was a simple timeout - relanding with requestLongerTimeout(2) in the test.

	Pulsebot
	Comment 10 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/22f20fcd23f9

	Phil Ringnalda (:philor)
	Comment 11 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/22f20fcd23f9

	Vasilica Tamas, QA [:vtamas] Reporter
	Comment 12 • 8 years ago

I was able to reproduce this issue on Firefox 46.0a1 using Windows 10 64-bit.

Verified fixed on Firefox 47.0a1 (2016-02-28) under Windows 10 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.11. The Synced Tabs panel is successfully closed while opening the “Firefox for Android” or “Firefox for iOS” link.