Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555783002:
[Mac] Ensure Omnibox text is always right-aligned in RTL (Closed)
DescriptionThere's a bunch of weird glitches right now that cause the text to flip to the left. AFAICT, these two changes fix all of them:
1) Set the alignment of the text field directly.
2) RFC 3987 requires us to set the writing direction of URLs to LTR so that we get שלום.com instead of moc.שלום. This appears to change the alignment of the string if it's not set explicitly. So: set it explicitly.
BUG=648554, 673362
Committed: https://crrev.com/4f56da4755ccc370cd682cce472d4ba1c0781207
Cr-Commit-Position: refs/heads/master@{#438529}
Patch Set 1 #Patch Set 2 : Doing this right #Patch Set 3 : Tweaks #
Total comments: 2
Patch Set 4 : Friend tests instead of exposing private method #
Total comments: 6
Patch Set 5 : Review comments #
Total comments: 1
Patch Set 6 : Add more Omnibox plumbing in text and remove |CurrentTextIsURL| mock plumbing #Patch Set 7 : User ScopedForceRTLMac for RTL test setup #
Messages
Total messages: 24 (10 generated)
Description was changed from ========== [Mac] Don't force URLs to LTR writing direction Tried a bunch of the examples in https://www.w3.org/International/iri-edit/BidiExamples and they're consistent with Views in both RTL and LTR so I don't think we need this code for RFC 3987 compliance. It does, however, cause many of the glitches with the Omnibox randomly flipping to be left-aligned. See #3 and #5 in the attached bug. This change keeps the text in the Omnibox right aligned at all times, except one case I haven't been able to nail down: on first load, when pressing delete to clear the selected URL. BUG=648554 ========== to ========== There's a bunch of weird glitches right now that cause the text to flip to the left. AFAICT, these two changes fix all of them: 1) Set the alignment of the text field directly. 2) RFC 3987 requires us to set the writing direction of URLs to LTR so that we get שלום.com instead of moc.שלום. This appears to change the alignment of the string if it's not set explicitly. So: set it explicitly. BUG=648554 ==========
lgrey@chromium.org changed reviewers: + ellyjones@chromium.org
PTAL :)
lgtm https://codereview.chromium.org/2555783002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2555783002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:134: // Public for testing. It's better to friend the test classes I think
lgrey@chromium.org changed reviewers: + mark@chromium.org
Thanks! Mark PTAL for OWNERS https://codereview.chromium.org/2555783002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2555783002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:134: // Public for testing. On 2016/12/09 18:56:16, Elly Fong-Jones wrote: > It's better to friend the test classes I think Done.
LGTM https://codereview.chromium.org/2555783002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2555783002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:136: FRIEND_TEST_ALL_PREFIXES(OmniboxViewMacTest, #include "base/gtest_prod_util.h" for this macro. https://codereview.chromium.org/2555783002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:186: void ApplyTextStyle(NSMutableAttributedString* attributedString); I don’t see why this needs to move. https://codereview.chromium.org/2555783002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm (right): https://codereview.chromium.org/2555783002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm:39: up_or_down_count_(0) {} , currrent_text_is_url_(false)
lgrey@chromium.org changed reviewers: + pkasting@chromium.org
Thanks, Mark! +pkasting@ for components/omnibox/browser/ https://codereview.chromium.org/2555783002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2555783002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:136: FRIEND_TEST_ALL_PREFIXES(OmniboxViewMacTest, On 2016/12/12 17:45:24, Mark Mentovai wrote: > #include "base/gtest_prod_util.h" for this macro. Done. https://codereview.chromium.org/2555783002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:186: void ApplyTextStyle(NSMutableAttributedString* attributedString); On 2016/12/12 17:45:24, Mark Mentovai wrote: > I don’t see why this needs to move. Accidentally put it back in the wrong place after adding the friend thing. Done. https://codereview.chromium.org/2555783002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm (right): https://codereview.chromium.org/2555783002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm:39: up_or_down_count_(0) {} On 2016/12/12 17:45:24, Mark Mentovai wrote: > , currrent_text_is_url_(false) Done.
Description was changed from ========== There's a bunch of weird glitches right now that cause the text to flip to the left. AFAICT, these two changes fix all of them: 1) Set the alignment of the text field directly. 2) RFC 3987 requires us to set the writing direction of URLs to LTR so that we get שלום.com instead of moc.שלום. This appears to change the alignment of the string if it's not set explicitly. So: set it explicitly. BUG=648554 ========== to ========== There's a bunch of weird glitches right now that cause the text to flip to the left. AFAICT, these two changes fix all of them: 1) Set the alignment of the text field directly. 2) RFC 3987 requires us to set the writing direction of URLs to LTR so that we get שלום.com instead of moc.שלום. This appears to change the alignment of the string if it's not set explicitly. So: set it explicitly. BUG=648554, 673362 ==========
https://codereview.chromium.org/2555783002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm (right): https://codereview.chromium.org/2555783002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm:44: bool CurrentTextIsURL() const override { return current_text_is_url_; } Why are you adding this override instead of simply using a real URL as the text below, which will cause this to work correctly automatically?
On 2016/12/12 19:57:10, Peter Kasting wrote: > https://codereview.chromium.org/2555783002/diff/80001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm (right): > > https://codereview.chromium.org/2555783002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm:44: bool > CurrentTextIsURL() const override { return current_text_is_url_; } > Why are you adding this override instead of simply using a real URL as the text > below, which will cause this to work correctly automatically? Tried to do this initially, but it would require mocking out a lot more of the Omnibox
On 2016/12/12 20:00:34, lgrey wrote: > On 2016/12/12 19:57:10, Peter Kasting wrote: > > > https://codereview.chromium.org/2555783002/diff/80001/chrome/browser/ui/cocoa... > > File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm (right): > > > > > https://codereview.chromium.org/2555783002/diff/80001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm:44: bool > > CurrentTextIsURL() const override { return current_text_is_url_; } > > Why are you adding this override instead of simply using a real URL as the > text > > below, which will cause this to work correctly automatically? > > Tried to do this initially, but it would require mocking out a lot more of the > Omnibox Like what? I'd much rather have the test code add more code, for example, than touch the edit model API in this way. (Note: In the future, try to reply to inline comments using more inline comments in the codereview tool rather than by email or with the "send message" box -- that keeps the comments together so it's easier to trace individual replies when spelunking later to figure out a change.)
On 2016/12/12 20:14:12, Peter Kasting wrote: > On 2016/12/12 20:00:34, lgrey wrote: > > On 2016/12/12 19:57:10, Peter Kasting wrote: > > > > > > https://codereview.chromium.org/2555783002/diff/80001/chrome/browser/ui/cocoa... > > > File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm (right): > > > > > > > > > https://codereview.chromium.org/2555783002/diff/80001/chrome/browser/ui/cocoa... > > > chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm:44: bool > > > CurrentTextIsURL() const override { return current_text_is_url_; } > > > Why are you adding this override instead of simply using a real URL as the > > text > > > below, which will cause this to work correctly automatically? > > > > Tried to do this initially, but it would require mocking out a lot more of the > > Omnibox > > Like what? I'd much rather have the test code add more code, for example, than > touch the edit model API in this way. > > (Note: In the future, try to reply to inline comments using more inline comments > in the codereview tool rather than by email or with the "send message" box -- > that keeps the comments together so it's easier to trace individual replies when > spelunking later to figure out a change.) I got as far as hooking up an edit controller and toolbar model, which is enough to allow setting text on the model or view without crashing, but not enough to get the state into the right place. I think some amount of the autocomplete stuff would need to get involved, but I don't know this code very well, so it's hard for me to say. Here's a stack trace of where the disconnect occurs: https://paste.googleplex.com/6037042018385920
Updated PTAL. Removed |CurrentTextIsURL| mocking. Thanks for working this out with me last night, pkasting@
Still LGTM
The CQ bit was checked by lgrey@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ellyjones@chromium.org Link to the patchset: https://codereview.chromium.org/2555783002/#ps120001 (title: "User ScopedForceRTLMac for RTL test setup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481731815211620,
"parent_rev": "932e87847f393f1409fb99e094dd8b821700f633", "commit_rev":
"61776530c4ba65536b690bf3de6a4f3738a9ebe3"}
Message was sent while issue was closed.
Description was changed from ========== There's a bunch of weird glitches right now that cause the text to flip to the left. AFAICT, these two changes fix all of them: 1) Set the alignment of the text field directly. 2) RFC 3987 requires us to set the writing direction of URLs to LTR so that we get שלום.com instead of moc.שלום. This appears to change the alignment of the string if it's not set explicitly. So: set it explicitly. BUG=648554, 673362 ========== to ========== There's a bunch of weird glitches right now that cause the text to flip to the left. AFAICT, these two changes fix all of them: 1) Set the alignment of the text field directly. 2) RFC 3987 requires us to set the writing direction of URLs to LTR so that we get שלום.com instead of moc.שלום. This appears to change the alignment of the string if it's not set explicitly. So: set it explicitly. BUG=648554, 673362 Review-Url: https://codereview.chromium.org/2555783002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== There's a bunch of weird glitches right now that cause the text to flip to the left. AFAICT, these two changes fix all of them: 1) Set the alignment of the text field directly. 2) RFC 3987 requires us to set the writing direction of URLs to LTR so that we get שלום.com instead of moc.שלום. This appears to change the alignment of the string if it's not set explicitly. So: set it explicitly. BUG=648554, 673362 Review-Url: https://codereview.chromium.org/2555783002 ========== to ========== There's a bunch of weird glitches right now that cause the text to flip to the left. AFAICT, these two changes fix all of them: 1) Set the alignment of the text field directly. 2) RFC 3987 requires us to set the writing direction of URLs to LTR so that we get שלום.com instead of moc.שלום. This appears to change the alignment of the string if it's not set explicitly. So: set it explicitly. BUG=648554, 673362 Committed: https://crrev.com/4f56da4755ccc370cd682cce472d4ba1c0781207 Cr-Commit-Position: refs/heads/master@{#438529} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4f56da4755ccc370cd682cce472d4ba1c0781207 Cr-Commit-Position: refs/heads/master@{#438529} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
