https://codereview.chromium.org/2626333002/ contains proposed patch

Menu for links with proposed patch contains title (in bold) + link; This is one scrollable element

Deleted: Screenshot_20170112-215319[1].png 193 KB

	Screenshot_20170112-215319[1].png 193 KB View Download

Menu for javascript links with proposed patch doesn't have unnecessary options

Deleted: Screenshot_20170112-215305[1].png 207 KB

	Screenshot_20170112-215305[1].png 207 KB View Download

Menu for pictures with proposed patch contains title + link (or picture text if this is data:image)

Deleted: Screenshot_20170112-215311[1].png 200 KB

	Screenshot_20170112-215311[1].png 200 KB View Download

URL with proposed patch is more clear (in the example contains "Łąka" instead of "%....")

Deleted: Screenshot_20170112-215501[1].png 462 KB

	Screenshot_20170112-215501[1].png 462 KB View Download

it looks, that part of these changes was implemented some time ago: https://bugs.chromium.org/p/chromium/issues/detail?id=487782

Assigning to ktam@ as frontend PM, adding rolfe@ because she controls the UX and needs to sign off on what you're proposing. 

As with last time, these kinds of things need to be discussed before you send off patches to (1) prevent spending engineering effort on something that potentially can't land and (2) prevent butting against other in-progress projects that are being done to redesign the context menu.

(For internal reference: crbug.com/655359 has links to the new context menu mockups but they're all inaccessible to external contributors)

Thank you, are ktam@ and rolfe@ responsible for the whole UI ? 

No, but they're the main points of contact for this kind of thing.  The UX team has a bunch of people working on different parts of the browser; bugs can be rerouted to the right people as necessary.

+bbergher who has been working on context menu redesigns

Thanks for your feedback. To respond specifically:

1. displaying URLs with decoded '%'-escaped characters
> that seems reasonable to me

2. displaying link text/picture title to allow user fast understanding if correct element was clicked
> this is something we are addressing with the context menu redesign

3. disabling useless options (for javascript: links)
> I'd be fine with this if there's no unexpected knock-on effects from this change.

4. removing unnecessary padding
> also being addressed with the context menu redesign

I have created https://bugs.chromium.org/p/chromium/issues/detail?id=682393 about displaying URLs with decoded '%'-escaped characters also in the popup info menu. Can be it approved ?

And when can I get any update what from my patch could be/should be applied ?

Hi,

friendly ping -> any news here?

Sounds like security team would have a look at the '%' proposal since there's apparently potential issues stemming from allowing unicode.

In addition, I'd scope down the bug to focus on decoding % and disabling javascript: options. +jwanda who's also looking into it.

> Sounds like security team would have a look at the '%' 
> proposal since there's apparently potential issues 
> stemming from allowing unicode.
If Uri.decode is non safe, that yes -> we have issue and Uri.decode
is place for fixes :)

> In addition, I'd scope down the bug to focus 
> on decoding % and disabling javascript: options.
> +jwanda who's also looking into it.

??? Right now we don't have any "javascript:" handling in code

Once again: can we maybe look into patch and look, what unsafe or wrong can be there ? we have concrete change proposed, not academic discussion (I don't want
this to sound in-polite, just kind request)

> > In addition, I'd scope down the bug to focus 
> > on decoding % and disabling javascript: options.
> > +jwanda who's also looking into it.

> ??? Right now we don't have any "javascript:" handling in code
I apologize -> menu options are disabled in this situation:

if (TextUtils.isEmpty(params.getLinkUrl()) || params.getLinkUrl().equals(BLANK_URL)) {

nothing new is in fact done, Chrome already handles, in which situation it should happen (when this url is empty) and I just use it

Thanks ktam for the cc!

I took a bit of time to look into this request. So there could be potential problems with transforming unicode. For example there is a code for "null" right now. How do we transform that link? (This is actually something that happened two years ago, Tom Scott made an awesome video about it: https://youtu.be/0fw5Cyh21TE)  How about when a user copies and pastes the link and possibly sees discrepancies between the link and the text?

This is definitely interesting and we'll continue to look into this.

> I took a bit of time to look into this request.
> So there could be potential problems with transforming unicode. 
> For example there is a code for "null" right now.
> How do we transform that link? (This is actually something
> that happened two years ago,
> Tom Scott made an awesome video about it: https://youtu.be/0fw5Cyh21TE)

https://developer.android.com/reference/android/net/Uri.html#decode(java.lang.String)

thx for video

> How about when a user copies and pastes the link and 
> possibly sees discrepancies between the link and the text?

hmmm, it already happens for this menu: for R.id.contextmenu_save_link_as we use  getUnfilteredLinkUrl(); field

> This is definitely interesting and we'll continue to look into this.

thx a lot

hi,

can I ask for any decision/comment/update here ?

hi,

We have started Jan 12 and now seven people are involved. Patch doesn't change very deeply internals and is mainly better formatting some info to user using already prepared Chrome code and Uri.decode (API 1).

I don't see in fact very strong voice against, can we have LGTM ?

I'd like someone from security (Matt?) to sign off given URL decoding is a tricky topic. It does appear that if you hover over a URL encoded link on desktop, the URL in the tooltip is decoded.

In general, I'd like to refrain from changing the UI in ways that won't carry forward in the redesign since it will be temporary and add more churn/review complexity.

1. displaying URLs with decoded '%'-escaped characters
> This seems like a good behavior to bring forward barring no security risk.

2. displaying link text/picture title to allow user fast understanding if correct element was clicked
> This is something I think we should avoid changing for now since it's something we're addressing in the redesign.

3. disabling useless options (for javascript: links)
> This is something I'd be happy to add now.

4. removing unnecessary padding
> Ditto with #2.

> I'd like someone from security (Matt?)
> to sign off given URL decoding is a tricky topic. 
> It does appear that if you hover over a URL encoded
> link on desktop, the URL in the tooltip is decoded.

well, there is bug https://bugs.chromium.org/p/chromium/issues/detail?id=682393, where I was asked for proving that it's safe although I'm using function available since API 1 and ALREADY used in Android Chrome. The problem is that nobody specified what kind of proof is expected.

> In general, I'd like to refrain from changing the UI 
> in ways that won't carry forward in the redesign 
> since it will be temporary and add more churn/review complexity.

hmmm, when you will look into my patch, you will see, that it's few lines. I don't know when redesign will be done, but if after few months, that in these months menu would look much better than currently.

1. displaying URLs with decoded '%'-escaped characters
> This seems like a good behavior to bring forward barring no security risk.

2. displaying link text/picture title to allow user fast understanding if correct element was clicked
> This is something I think we should avoid changing for now since it's something we're addressing in the redesign.

First - only few lines of code

Second - maybe redesign is not required after applying patch?

3. disabling useless options (for javascript: links)
> This is something I'd be happy to add now.

4. removing unnecessary padding
> Ditto with #2.

Addressed the decoding issue in the other bug. As for visual changes, we would need to have additional UX look at it since it may have unexpected side-effects/involves multiple use cases to test and I think it's best to keep it kept as-is for now since it will be updated in the redesign.

> Addressed the decoding issue in the other bug.

True 

> As for visual changes, we would need to have additional UX
> look at it since it may have unexpected side-effects/involves
> multiple use cases to test and I think it's best to keep
> it kept as-is for now since it will be updated in the redesign.

Well, is it possible to get any concretes ? 

I proposed relatively some small change on Jan 12 & today it's Feb 16 (so, it's more than month) and it's not even clear, when redesign is scheduled/targeted. 

It's really much easier to use browser when menu contains useful info and my change isn't revolution. Really.

Any updates here?

I have splitted patch:

1. contains on the top URL with encoded '%'-escaped characters (which is not very readable) - https://codereview.chromium.org/2774143002/

2. doesn't show in clear way, what element on the page was clicked - https://codereview.chromium.org/2778613002/

3. for javascript: links contains links "Open in new tab", "Open in incognito tab", "Open in other window" (which are useless) - https://codereview.chromium.org/2778603002/

4. contains unnecessary padding below URL - https://codereview.chromium.org/2779473002/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ebe1a464baa4fc3e460870ab83f568b2810108ec

commit ebe1a464baa4fc3e460870ab83f568b2810108ec
Author: marcin <marcin@mwiacek.com>
Date: Tue Mar 28 06:52:20 2017

Remove Open in new window/tab/incognito in ContextMenu for empty URL

BUG= 680673 

Review-Url: https://codereview.chromium.org/2778603002
Cr-Commit-Position: refs/heads/master@{#460031}

[modify] https://crrev.com/ebe1a464baa4fc3e460870ab83f568b2810108ec/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java

Removing myself. If core Android design input is needed, reach out to cleer@ but bbergher@ (already on the thread) is a good reference for the upcoming menu design too.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d8f6b4a8674574b435114e40b240af7741922f37

commit d8f6b4a8674574b435114e40b240af7741922f37
Author: marcin <marcin@mwiacek.com>
Date: Thu Mar 30 07:39:05 2017

Remove Open in new... in ContextMenu for empty URL (after 2778603002)

Required because 2778603002 was not enough

BUG= 680673 

Review-Url: https://codereview.chromium.org/2782723003
Cr-Commit-Position: refs/heads/master@{#460689}

[modify] https://crrev.com/d8f6b4a8674574b435114e40b240af7741922f37/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java

Closing out as we roll out the new context menu designs. (FYI feel free to test it out with the "Enable custom context menu" flag in chrome://flags)

1. displaying URLs with decoded '%'-escaped characters -> not done with new design (checked with "Google łąka")

2. displaying link text/picture title to allow user fast understanding if correct element was clicked -> not done with new design (we have only URL, not link text)

1. displaying URLs with decoded '%'-escaped characters
> Ah yes - I believe you have a separate bug for that somewhere?

2. displaying link text/picture title to allow user fast understanding if correct element was clicked
> We show a mini version of the image which I think should be sufficient. I haven't seen anything that would indicate we need link text though (especially with the new animation).

>> 1. displaying URLs with decoded '%'-escaped characters
> Ah yes - I believe you have a separate bug for that somewhere?

Yes, but there is no big progress with it since longer time

>> 2. displaying link text/picture title to allow user fast understanding if 
>> correct element was clicked
> We show a mini version of the image which I think should be sufficient. I haven't seen anything that would indicate we need link text though (especially with the new animation).

you can have 

(1) <a href=url>name</a> or

(2) <a href=url><img src=src title=title>name</a>

With new menu you display title and show picture (GOOD!) and show URL (GOOD!), but name is missed. Especially in (1) this is not very useful to see long url http://fkdhfkjdhfohiufhbvfikfubvikdfhvbikfuhvoiuhrftvor and nothing about link (no indication if you clicked "link A" or "link B")

Please look into product from ordinary user perspective - he's interested in first place to see if he clicked correct link, not about url or other funny things

1. I'd ping that bug again to see if any progress has been made there.

2. The name would add more complexity to the dialog especially if it's a long text e.g. "Bondhus 10999 Set of 9 Balldriver L-wrenches, ProGuard Finish, sizes 1.5-10mm". As we already want the URL for security reasons and have an animation, I think it's sufficient.

Issue 654354 has been merged into this issue.