Sharing link to Samsung messages app is done twice |
||||||
Issue descriptionSteps to reproduce the problem: 1. try to share link from new context menu to Samsung messages app What is the expected behavior? Link is shared once What went wrong? Link is shared twice, in this concrete case workaround is probably required. Did this work before? No Chrome version: 65.0.3309.0 Channel: canary OS Version: 7 Flash Version: S7, Android 7
,
Jan 3 2018
Tested the issue in Android and able to reproduce the issue. Steps Followed: 1. Launched the Chrome Browser on Samsung mobile. 2. Navigated to cnn.com 3. Long tap on any link. 4. Tap on Share Link option and try sharing through default messaging app. 5. Observed that link is pasted twice in the message. Chrome versions tested: 65.0.3309.0(Canary) OS: Android 7.0.0 Android Devices: J710F Build/NRD90M This seems to be a Non-Regression issue as issue is observed on the latest Feature. Untriaged for further input's on this issue. Please navigate to below link for log's and video-- go/chrome-androidlogs/798323 Note: This issue is not observed in Desktop.
,
Jan 3 2018
+Ted not quite sure what is going on here but CC'ing Ted who might know more
,
Jan 3 2018
If this only affects the samsung messaging app, I'd argue that it is likely an app issue on their end. With that said, we should see if we're sending duplicate data. I suspect we're sending the URL as the Intent.EXTRA_SUBJECT and Intent.EXTRA_TEXT. We could see if we should drop one of those if they are the same value, but we'd need to verify it doesn't break anything else.
,
Jan 3 2018
For link share in context menu, we set all title, text and url the same value which is the url of the link. This makes the Intent.EXTRA_SUBJECT and Intent.EXTRA_TEXT have the same value. I am concerned that if we drop any of these two, there might be the problem for other apps who expects both values from share intent? Because those two are pretty common fields for share intent.
,
Jan 5 2018
What can be done about this? Can be Chrome interface redesigned? Can we take list of apps, display them in Chrome and treat separately/differently? Samsung will probably not update all devices on the market.
,
Jan 25 2018
Before any change lands in Chrome, someone needs to do the investigation with sending these share intents with just one of the fields to make sure no app breaks. If we get a matrix of the top apps and ensure they work as expected, then we could proceed with making this change in Chrome.
,
Jan 25 2018
marcin@ has a proposed CL here (https://chromium-review.googlesource.com/c/chromium/src/+/885819). I agree we should test the behavior out across various apps. Additionally we can test sharing from other apps to the Samsung Messaging app as well and see what they set to try to understand how the APIs are being used.
,
Jan 25 2018
For me in this thread there are unfortunately visible two things: it seems, that nobody considered patch solution / code (using two fields - one empty in some cases) and patch received system "doesn't look good to me" although there are no technical reasons for it. This is sad especially that investigation was done, patch prepared and now investigation will be done again (because of it ready solution will not land for Chrome for short or long time). I lost time, you loose time, users don't get solution. Not good.
,
Jan 25 2018
What investigation did you do? Did you post it anywhere? Did you highlight the compatibility with any apps or other browsers? If you want to be engaged in the open source community, you need to show the due diligence to make these changes. All we see is, "Hey, I want this to be different", I'm going to upload the patch and you deal with the consequences. Prove to us that this is what we should do. If not, we won't except it. If you do not want to work this way, please find another project to contribute to. I'd recommend spending time reading this thoroughly and only attempting to deal with chromium once you accept this is how we work: http://dev.chromium.org/developers/contributing-code#TOC-Communicate
,
Jan 25 2018
CODE_OF_CONDUCT.md: "Simply put, community discussion should be: [...] * about features and code" I haven't see any comments to code, if you expect proof, please describe how it should look like. Yes, I can write - I have tested it with Samsung messaging and some others. Do you expect list or screenshots? (IMHO, this is role as reviewer to look into patch and define what you think about it) CODE_OF_CONDUCT.md: "Be respectful and constructive. [...] Build on each other's ideas" I don't see any discussion about solution proposed in patch (sending empty EXTRA_SUBJECT when it's equal to EXTRA_TEXT). Do you have any questions or is there any technical reason to mark patch as useless? If I missed something from my side, I'm sorry.
,
Jan 25 2018
BTW, I engaged into open source community since 1999. A lot of time and a lot of different behaviors inside different teams. Let's move this topic, but please don't waste time especially that ready working and probably the best code was proposed. I understand, that discussion should concentrate on it if we can find something better or not.
,
Jan 25 2018
It is 100% the expectation of the code author to verify that their change does not negatively impact anything. As stated above, this fix will only be considered if you show (screenshots, videos, list of all apps being tested) to verify it doesn't break. We are not going to blindly accept a patch or take the burden of justify your patch works just because you want to land this. It appears that you did not read the link I sent before, so let me highlight the critical points: "If it is in the existing code base, it is a good idea to talk to some of the folks in the "OWNERS" file (see code review policies for more) for the code you want to change." "Keep in mind that just because there is a bug in the bug system doesn't necessarily mean that a patch would be accepted." The burden of proof is on you. You can be constructive and proactive, or defensive and aggressive. The latter will not get you very far and quickly will ensure no one will even consider your patches because of your communication style. How you should have approached this, "Hey, I tested this change by sharing to these X apps, I also tested these Y browsers and confirmed their behavior is the same. Is there anything else you want me to test on before proceeding?" That would have been met with a positive response and a willingness to work with you. If you can adjust your style towards this helpful, productive attitude, then we will welcome your contributions to Chromium. If you do not want to do this leg work, we will get to it eventually and do it before attempting to upload any code.
,
Jan 25 2018
I understand, that discussion should be continued here. You're right with some things, maybe my bad, that I assumed, that such trivial change doesn't need so deep explaining. Just one final note from my side: some behaviors are not happening from nothing and I've commented after receiving hard "-1" for patch. Anyway, I've tested patch with Samsung Messages, WhatsApp, Gmail, Add to Firefox Broswer, Email, Fast Notepad, Skype, should I test with some others? I'm attaching some screenshots, I hope, that it's enough especially that I'm sending both fields and patch is not changing anything deep in fact.
,
Jan 25 2018
Hey Marcin thanks for kicking off the investigation! Just to be clear, could you confirm that: - Sending only the EXTRA_TEXT works as expected with some popular apps (Facebook, Samsung Messages, WhatsApp, Gmail, etc. as you listed). - Other browsers only send EXTRA_TEXT and not EXTRA_SUBJECT (and if they send both, are they also broken on Samsung Messages?). Thanks!
,
Jan 25 2018
Hey David, You welcome, I love when things are going fast forward and I don't like showstoppers, negative emotions and sometimes doing art for art. I can confirm, that some apps are separating SUBJECT and TEXT (for example it's visible in email clients), some merging (like Samsung Messages) and some are ignoring SUBJECT at all. I can also confirm, that FireFox seems to give the same results like Chrome with my fix - it shares page name/url into correct places in email apps (and they're merged in Samsung Messages) and it shares only url, when page name is not available. I don't want to loose this functionality and additionally I want to decrease possibility of breaking something (that's why EXTRA_SUBJECT is sometimes empty with patch). If this explanation is not enough, I will try only EXTRA_TEXT (but I can do it later).
,
Jan 29 2018
Friendly ping - do you need checking only EXTRA_TEXT after explanations from #17 ?
,
Jan 29 2018
Thanks for the investigation! I actually think it makes sense to do only EXTRA_TEXT and leave EXTRA_SUBJECT blank if they're the same. This will probably make the email experience better because subjects are often not clickable on mobile email clients. Can you do a quick check to make sure this work as expected? I think we're close to closing this out. Good stuff!
,
Jan 29 2018
> Thanks for the investigation! Thank you for discussion and help > I actually think it makes sense to do only EXTRA_TEXT and > leave EXTRA_SUBJECT blank if they're the same. > This will probably make the email experience > better because subjects are often not clickable on mobile email clients. This is exactly what patch is doing (from patch description -> "we send empty EXTRA_SUBJECT when it's equal to EXTRA_TEXT.") > Can you do a quick check to make sure this work as expected? From #14: "I've tested patch with Samsung Messages, WhatsApp, Gmail, Add to Firefox Broswer, Email, Fast Notepad, Skype, should I test with some others? I'm attaching some screenshots, I hope, that it's enough" > I think we're close to closing this out. Good stuff! I'm very glad and happy, thank you.
,
Feb 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b92ff83f4a789be8b96e4bc5e5283b7b96030187 commit b92ff83f4a789be8b96e4bc5e5283b7b96030187 Author: marcin <marcin@mwiacek.com> Date: Sun Feb 04 23:27:20 2018 Sending EXTRA_SUBJECT during sharing only when != EXTRA_TEXT Current: we're always sending EXTRA_SUBJECT and EXTRA_TEXT during sharing links. New: sending EXTRA_SUBJECT during sharing only when it's not equal to EXTRA_TEXT It will make sharing more user friendly (in some apps like Samsung messaging app user will not see the same text twice) Bug:798323 Change-Id: I6e478d494004f16763cf826a6aaddba021326443 Reviewed-on: https://chromium-review.googlesource.com/885819 Reviewed-by: David Trainor <dtrainor@chromium.org> Reviewed-by: Ted Choc <tedchoc@chromium.org> Commit-Queue: Marcin WiÄ…cek <marcin@mwiacek.com> Cr-Commit-Position: refs/heads/master@{#534306} [modify] https://crrev.com/b92ff83f4a789be8b96e4bc5e5283b7b96030187/chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
,
Feb 8 2018
I will mark it as fixed and change owner to me because of fix, please change it again if you think that it's incorrect.
,
Feb 8 2018
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by pnangunoori@chromium.org
, Jan 3 2018