Closed Bug 1237945 Opened 6 years ago Closed 6 years ago

Synced Tabs panel is not closed while Android/iOS pages are opened

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
firefox45 --- affected
firefox46 --- affected
firefox47 --- verified

People

(Reporter: vtamas, Assigned: markh)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 1239084
Priority: -- → P3
Priority: P3 → P1
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.
Assignee: nobody → markh
Status: NEW → ASSIGNED
Attachment #8709749 - Flags: feedback?(gijskruitbosch+bugs)
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.
Attachment #8709749 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
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.
Attachment #8709749 - Attachment is obsolete: true
Attachment #8710268 - Flags: review?(gijskruitbosch+bugs)
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...
Attachment #8710268 - Flags: review?(gijskruitbosch+bugs)
(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!
Attachment #8710268 - Attachment is obsolete: true
Attachment #8710864 - Flags: review?(gijskruitbosch+bugs)
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...
Attachment #8710864 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/22f20fcd23f9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.