Closed
Bug 1253604
Opened 8 years ago
Closed 8 years ago
Share functionality ceases to work correctly after canceling a Facebook post using the "Share This Page" option
Categories
(Firefox Graveyard :: SocialAPI, defect)
Tracking
(firefox47 affected, firefox48 fixed, firefox49 fixed)
VERIFIED
FIXED
Firefox 49
People
(Reporter: cmuresan, Assigned: mixedpuppy)
References
Details
Attachments
(2 files, 4 obsolete files)
3.10 MB,
video/mp4
|
Details | |
18.24 KB,
patch
|
mixedpuppy
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
[Affected versions]: - Firefox 44.0.2 - Firefox 45beta10 - Latest Aurora 46.0a2, Build ID 20160304004008 - Latest Nightly 47.0a1, Build ID 20160303030253 [Affected platforms]: - Windows 7 x32/64, 8.1 x32, 10 x64 - Mac OS X 10.11 - Ubuntu 14.04 [Prerequisites]: - Add the Share This Page feature to the Menu (if not already added) from customization menu. [Steps to reproduce]: 1. Open the browser and navigate to a site. 2. Right click on the page and choose "Share This Page" option. 3. Add the Facebook service and log in. 4. In the Facebook Post form click "Cancel" button. 5. When the "Are you sure?" pop-up is displayed, Click "Leave Page" button. 6. Right click again on the page and select "Share This Page" option. 7. Observe the browser behavior. [Expected result]: - The container is opened and populated with a Facebook Post form containing the page in question. [Actual result]: - A new tab is opened at the same time the container is displayed. The tab contains all the information that should be in the container. - Clicking anywhere on the page opens an empty tab with "data:text/plain;charset=utf8," autocompleted in the URL bar. - After this the Share functionality no longer works with any service. [Regression range]: - This doesn't seem to be a regression as the issue is reproducible on Nightly builds from 2014-10-10. Builds before this date do not have the Share feature. [Additional notes]: - The Share functionality works correctly only after Firefox has been restarted.
Assignee | ||
Comment 1•8 years ago
|
||
I don't understand WHY this is happening, but this does fix the problem. I'm taking a guess that the "stay on page" dialog is interfering somehow. In any case, I do believe that the removed call is not necessary to unload the page, we were being overly aggressive with that.
Attachment #8726958 -
Flags: review?(markh)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mixedpuppy
Comment 2•8 years ago
|
||
Comment on attachment 8726958 [details] [diff] [review] fix share after cancelling on facebook Review of attachment 8726958 [details] [diff] [review]: ----------------------------------------------------------------- This change worries me - bug 944219 added a similar call to avoid leaks, and I seem to remember that the unload didn't automatically end up happening due to the item being hidden. I think we should try and better understand what is going on before we potentially allow the frame to not die.
Assignee | ||
Comment 3•8 years ago
|
||
Here's a more appropriate fix, prevents the beforeunload dialogs which are bad ux in this case anyway. The dialog was appearing while the document was also being destroyed in the panel onhidden.
Attachment #8726958 -
Attachment is obsolete: true
Attachment #8726958 -
Flags: review?(markh)
Attachment #8730287 -
Flags: review?(markh)
Comment 4•8 years ago
|
||
Comment on attachment 8730287 [details] [diff] [review] fix share after cancelling on facebook Review of attachment 8730287 [details] [diff] [review]: ----------------------------------------------------------------- This looks like a much better approach, any chance of tests? I'm also wondering if the same problem is going to exist for other bits of the social UI? Facebook is showing this for the share panel, but I don't see why the sidebar etc wouldn't exhibit the same behaviour if a provider tried - tests might also help determine if that's possible or not. If that does turn out to be true, it seems like the message could be more generic - something like Social:InitializeSocialFrame or something...
Assignee | ||
Comment 5•8 years ago
|
||
Tests already exist for checking beforeunload with dialogs disabled in layout. I'd actually be fine doing this globally for all frames that load social-content.js, it could just be done in DOMContentLoaded (already have a listener for that). However that would also affect chatwindows for hello...Standard8, do you have any issue with that?
Flags: needinfo?(standard8)
Comment 6•8 years ago
|
||
I can't remember if I already tried to answer this or not. AFAIK we don't have an issue with blocking beforeUnload - we tend to just modify what the close/hangup buttons do.
Flags: needinfo?(standard8)
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd369e6ded2b
Assignee | ||
Comment 8•8 years ago
|
||
updates share tests to try onbeforeunload, also reenabling share tests (will see if window leaks)
Attachment #8730287 -
Attachment is obsolete: true
Attachment #8730287 -
Flags: review?(markh)
Attachment #8739594 -
Flags: review?(markh)
Comment 9•8 years ago
|
||
Comment on attachment 8739594 [details] [diff] [review] fix share after cancelling on facebook Review of attachment 8739594 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, but if possible I think we should be adding a new test file instead of changing this existing one (well, maybe the existing one needs a change, but listening for this event seems like it should remain. I guess you can ignore this if there are other existing tests for this event...) ::: browser/base/content/test/social/share.html @@ -1,5 @@ > <html> > <head> > <meta charset="utf-8"> > <script> > - addEventListener("OpenGraphData", function(e) { should we keep the event listener and add the new functionality as a new test?
Attachment #8739594 -
Flags: review?(markh) → review+
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fef0dc2fc77
Assignee | ||
Comment 11•8 years ago
|
||
This separates out the new test from the previously existing tests, fixes some async issues that likely were the cause of bug 1115131.
Attachment #8739594 -
Attachment is obsolete: true
Attachment #8744418 -
Flags: review?(markh)
Comment 12•8 years ago
|
||
Comment on attachment 8744418 [details] [diff] [review] fix share after cancelling on facebook Review of attachment 8744418 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks. ::: browser/base/content/test/social/browser_share.js @@ +367,5 @@ > + EventUtils.sendKey("f"); > + EventUtils.sendKey("a"); > + EventUtils.sendKey("i"); > + EventUtils.sendKey("l"); > + trailing whitespace
Attachment #8744418 -
Flags: review?(markh) → review+
Assignee | ||
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f1e0c231d7e49ea0f2e25eb3b64ed811d3614f81 Bug 1253604 share breakage after canceling share on fb, r=markh
Comment 14•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/fx-team/rev/f2b20c6eb71dbf7a2884a776251e760e73b8c69b There may be more things in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=5fdb0bf1527a446f946d3585371f4da0ab33c447 which were from this rather than from that push, I don't know, but at least the browser_share.js parts were from this, since https://treeherder.mozilla.org/logviewer.html#?job_id=8984029&repo=fx-team was on the push for this.
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7e9a3e920c5
Assignee | ||
Comment 16•8 years ago
|
||
change disables css animation during panel tests, fixes intermittent bug 1115131 that appeared during attempt to land. carry forward r+
Attachment #8744418 -
Attachment is obsolete: true
Attachment #8746067 -
Flags: review+
Assignee | ||
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/93c744e5c550c2265c29d5ef3c6fee4ddbcd6104 Bug 1253604 fix share breakage after canceling share on fb, r=markh
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/93c744e5c550
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8746067 [details] [diff] [review] fix share after cancelling on facebook Approval Request Comment [Feature/regressing bug #]:share [User impact if declined]:hitting cancel in facebook share panel breaks future attempts to share [Describe test coverage new/current, TreeHerder]:share tests had been disabled, patch fixes and reenables tests, adds test for this particular situation. [Risks and why]: low [String/UUID change made/needed]:none
Attachment #8746067 -
Flags: approval-mozilla-beta?
Attachment #8746067 -
Flags: approval-mozilla-aurora?
status-firefox48:
--- → affected
Hello Ciprian, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(ciprian.muresan)
Comment on attachment 8746067 [details] [diff] [review] fix share after cancelling on facebook Fixing fb share scenario (after it was canceled once), patch includes new automated test which is great, Aurora48+, Beta47+
Attachment #8746067 -
Flags: approval-mozilla-beta?
Attachment #8746067 -
Flags: approval-mozilla-beta+
Attachment #8746067 -
Flags: approval-mozilla-aurora?
Attachment #8746067 -
Flags: approval-mozilla-aurora+
Reporter | ||
Comment 22•8 years ago
|
||
Hi, I have retested the issue on the latest Nightly(49.0a1, Build ID 20160505030327) and I can reproduce the issue on my (possibly) dirty profile, by logging out of Facebook and logging back in step 3 of the original STR. I can also reproduce the issue with a new profile using the original STR. Tested on Windows 10 x64, Windows 7 x64, Ubuntu 14.04 x64 and Mac OS X 10.11 and the issue is reproducible on all platforms. However, after encountering the issue the first time on the new profile and restarting Firefox, the issue is no longer reproducible. But if I log out of Facebook and then log back in, the issue is again reproducible. I've followed the pushlog for the latest Nightly and the fix is surely in today's build. Any idea what might be causing this?
Flags: needinfo?(rkothari)
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(ciprian.muresan)
Comment 23•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/0123e9f41ced
I uplifted this to aurora despite comment 22. The patch had errors when I tried to apply it to beta, so didn't uplift it there.
Assignee | ||
Comment 25•8 years ago
|
||
Verified there is still a bug on this. A new page load (e.g. login via share panel then switches to share page) needs to re-disable the dialogs.
Flags: needinfo?(mixedpuppy)
Assignee | ||
Comment 26•8 years ago
|
||
A patch I've been working on for bug 1257723 actually fixes the new str, I'll try to pull out the share relevant fixes for this.
Comment on attachment 8746067 [details] [diff] [review] fix share after cancelling on facebook Reverting the Beta uplift flag as this fix is not complete. Please renom updated patch for uplift to Beta47.
Flags: needinfo?(rkothari)
Attachment #8746067 -
Flags: approval-mozilla-beta+ → approval-mozilla-beta-
(In reply to Ciprian Muresan [:cmuresan] from comment #22) > Hi, > > I have retested the issue on the latest Nightly(49.0a1, Build ID > 20160505030327) and I can reproduce the issue on my (possibly) dirty > profile, by logging out of Facebook and logging back in step 3 of the > original STR. > I can also reproduce the issue with a new profile using the original STR. > Tested on Windows 10 x64, Windows 7 x64, Ubuntu 14.04 x64 and Mac OS X 10.11 > and the issue is reproducible on all platforms. > > However, after encountering the issue the first time on the new profile and > restarting Firefox, the issue is no longer reproducible. But if I log out of > Facebook and then log back in, the issue is again reproducible. > > I've followed the pushlog for the latest Nightly and the fix is surely in > today's build. Any idea what might be causing this? Thank you for a prompt verification! It was extremely valuable. Our dev will be updating the patch with additional fix needed to handle the re-login scenario.
Assignee | ||
Comment 29•8 years ago
|
||
Making this depend on changes in bug 1257723 which addresses the issues I described in comment 25. If this bug uplifts, we'll need to uplift bug 1257723 also. For that reason, I'm going to skip on a fix for beta and only request uplift to aurora.
Depends on: 1257723
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
Comment 31•8 months ago
|
||
Marked as verified as the feature is no longer available in the current Firefox builds.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•