Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1189553002:
Omnibox: Force text field to LTR context if it is a URL. (Closed)
|
Created:
5 years, 6 months ago by Matt Giuca Modified:
5 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, James Su, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOmnibox: Force text field to LTR context if it is a URL.
This means that URLs will be displayed in a left-to-right paragraph
context. Right-to-left runs are still rendered RTL, but will not flip
the whole URL around. For example (if "ABC" is Hebrew), this will render
"ABC.com" as "CBA.com", rather than "com.CBA".
This is consistent with the behaviour in the Omnibox drop-down items
(OmniboxResultView::CreateClassifiedRenderText) and status bubble
(StatusBubbleViews::SetURL).
BUG=495933, 421332
Committed: https://crrev.com/1c7d9ce02925cf766fc508d4ee83424369e71548
Cr-Commit-Position: refs/heads/master@{#334537}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase. #Patch Set 3 : Rewrite comment with an example. #Messages
Total messages: 18 (5 generated)
mgiuca@chromium.org changed reviewers: + pkasting@chromium.org
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189553002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
LGTM https://codereview.chromium.org/1189553002/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/1189553002/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:609: // rendered on the left. This second sentence is a bit unclear. If you need to talk about how RTL text is rendered when the omnibox contents are a URL, consider giving a concrete example (i.e. a URL with RTL text in it, where this directionality forcing affects the rendering), instead of just being abstract.
https://codereview.chromium.org/1189553002/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): https://codereview.chromium.org/1189553002/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_views.cc:609: // rendered on the left. On 2015/06/15 22:39:24, Peter Kasting wrote: > This second sentence is a bit unclear. If you need to talk about how RTL text > is rendered when the omnibox contents are a URL, consider giving a concrete > example (i.e. a URL with RTL text in it, where this directionality forcing > affects the rendering), instead of just being abstract. Done.
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1189553002/#ps40001 (title: "Rewrite comment with an example.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189553002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1c7d9ce02925cf766fc508d4ee83424369e71548 Cr-Commit-Position: refs/heads/master@{#334537}
Message was sent while issue was closed.
So is there a testcase I can start typing that will show me how the text swaps around as I type and delete characters that flip between "is a URL" and "not a URL"? I LGTMed this on the technical implementation, but I'm worried about a jarring user experience when typing and I'd like to feel it out for myself.
Message was sent while issue was closed.
Yes, it is a legitimate concern. The main problem is that there seems to be some kind of heuristic to decide between picking a URL result or a Google Search result in the Omnibox. Whichever one ends up on top determines whether the text field "is a URL" or not. This tends to oscillate back and forth unpredictably. This is already a bit jarring (even without my patch), but you're right, this feels pretty weird when using bidi text. Here's an example: paste into the browser (without quotes) "שנב.בםצ/xyz" then hit the End key, and start typing a sentence consisting of ASCII letters and spaces, or ASCII letters and slashes. In both cases, it will oscillate between being a URL and not being a URL, which after my patch, flips the Hebrew bit from the left to the right side. So what can we do? I don't really want to revert this* because it's a security issue. Can we possibly make the Omnibox selection heuristic more stable? Look at a similar ASCII-only example: Type: "abc.def/ghi" and then type a sequence of letters, spaces and slashes. You will see it oscillating between Google Search and URL, which makes the result list flicker back and forth, and also the non-domain parts of the URL flickering between black and grey. The other solution would be to only force LTR when the URL is in "permanent" (confirmed) mode, not during editing. I tried that initially and I was dissatisfied with the fact that the URL looked different during editing to after confirmation. But that is an option. * I mean I'm happy to temporarily while we figure out what to do, but I don't want to leave this unfixed.
Message was sent while issue was closed.
On 2015/06/17 03:16:03, Matt Giuca wrote: > Type: "abc.def/ghi" and then type a sequence of letters, spaces and slashes. You > will see it oscillating between Google Search and URL, which makes the result > list flicker back and forth, and also the non-domain parts of the URL flickering > between black and grey. I think this is mostly because of bug 418009. I CCed you there and wrote up some thoughts. Mark is going on leave soon so it may make sense to have you try and take over his fix for that bug if he can't get to it.
Message was sent while issue was closed.
OK I've got a bit on my plate at the moment but I might be able to help out. In the mean time (talking about the M45 branch point), do you think we should revert this temporarily or just live with the flipping behaviour for now?
Message was sent while issue was closed.
On 2015/06/17 05:39:47, Matt Giuca wrote: > OK I've got a bit on my plate at the moment but I might be able to help out. > > In the mean time (talking about the M45 branch point), do you think we should > revert this temporarily or just live with the flipping behaviour for now? If we don't manage to land anything else, I probably would not ship this to 45. Even if we land a fix for that bug we may find the behavior still feels icky, but we'll see. It's a security issue, but it's also one we've had for all time, so I'm willing to take longer to get it right if need be. |
