Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15197003:
Fix RTL issues in javascript alerts. (Closed)
|
Created:
7 years, 7 months ago by Avi (use Gerrit) Modified:
7 years, 7 months ago CC:
chromium-reviews, sail+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix RTL issues in javascript alerts.
BUG=70806
TEST=as in bug
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202471
Patch Set 1 #
Total comments: 9
Patch Set 2 : cleaner #
Total comments: 4
Patch Set 3 : indent #Patch Set 4 : title too #Messages
Total messages: 15 (0 generated)
mark for review, xji and jeremy FYI. I attached images of how this looks at https://code.google.com/p/chromium/issues/detail?id=70806#c15
https://codereview.chromium.org/15197003/diff/1/chrome/browser/ui/cocoa/javas... File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right): https://codereview.chromium.org/15197003/diff/1/chrome/browser/ui/cocoa/javas... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:11: #include "base/mac/foundation_util.h" #import me. https://codereview.chromium.org/15197003/diff/1/chrome/browser/ui/cocoa/javas... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:147: NSString* informativeText = base::SysUTF16ToNSString(dialog_->message_text()); Outside of @interface, @implementation, etc., name_like_this, notLikeThis. https://codereview.chromium.org/15197003/diff/1/chrome/browser/ui/cocoa/javas... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:162: // Fix RTL dialogs. Can you avoid doing this if it’s not needed? Gate this whole block on base::i18n::StringContainsStrongRTLChars(dialog_->message_text()). https://codereview.chromium.org/15197003/diff/1/chrome/browser/ui/cocoa/javas... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:184: if (!textField) This check is unnecessary. https://codereview.chromium.org/15197003/diff/1/chrome/browser/ui/cocoa/javas... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:190: informativeTextField = textField; break here? Since you’ve removed one of the “continue”s, the loop would probably work better if you structured it as for each view { text field = cast(view) if (text field's string value is equal) { remember this field break } } https://codereview.chromium.org/15197003/diff/1/chrome/browser/ui/cocoa/javas... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:193: if (informativeTextField) { DCHECK(informativeTextField) ? https://codereview.chromium.org/15197003/diff/1/chrome/browser/ui/cocoa/javas... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:197: [[NSParagraphStyle defaultParagraphStyle] mutableCopy]); Nit: continuation line indent gets another 2 spaces. https://codereview.chromium.org/15197003/diff/1/chrome/browser/ui/cocoa/javas... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:199: [alignment setAlignment:NSRightTextAlignment]; Ideas: Ternary? Or avoid having an alignment attribute altogether if it’s LTR?
Addressed everything. https://codereview.chromium.org/15197003/diff/1/chrome/browser/ui/cocoa/javas... File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right): https://codereview.chromium.org/15197003/diff/1/chrome/browser/ui/cocoa/javas... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:199: [alignment setAlignment:NSRightTextAlignment]; On 2013/05/16 22:34:27, Mark Mentovai wrote: > Ideas: Ternary? Or avoid having an alignment attribute altogether if it’s LTR? Ternary. At this point I'm a little paranoid and just want to set things.
LGTM https://codereview.chromium.org/15197003/diff/5001/chrome/browser/ui/cocoa/ja... File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right): https://codereview.chromium.org/15197003/diff/5001/chrome/browser/ui/cocoa/ja... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:178: [alert_ layout]; The indentation is wrong on this line.
One remark, thank you so much for looking into this (!!!!) https://codereview.chromium.org/15197003/diff/5001/chrome/browser/ui/cocoa/ja... File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right): https://codereview.chromium.org/15197003/diff/5001/chrome/browser/ui/cocoa/ja... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:202: : NSLeftTextAlignment]; Do you also need to manually set the directionality of the string - NSWritingDirectionAttributeName?
LGTM otherwise.
https://codereview.chromium.org/15197003/diff/5001/chrome/browser/ui/cocoa/ja... File chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm (right): https://codereview.chromium.org/15197003/diff/5001/chrome/browser/ui/cocoa/ja... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:178: [alert_ layout]; On 2013/05/17 01:32:57, Mark Mentovai wrote: > The indentation is wrong on this line. Done. https://codereview.chromium.org/15197003/diff/5001/chrome/browser/ui/cocoa/ja... chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm:202: : NSLeftTextAlignment]; On 2013/05/19 10:51:50, jeremy wrote: > Do you also need to manually set the directionality of the string - > NSWritingDirectionAttributeName? It turns out that I don't need to. As soon as you set the attributed string, even if it has no attributes, the directionality magically starts working again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avi@chromium.org/15197003/14001
OK, this is weird. If you try to select the text, it shifts around. I need to look into this further.
Mark: The easy way to fix the whole everything-scrambles-on-select is to make the text unselectable. I think it's a fair compromise.
Non-selectable is fine with me. I actually don’t think that non-user-editable fields should ever be selectable (remember the About box discussions?) LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avi@chromium.org/15197003/25001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avi@chromium.org/15197003/25001
Message was sent while issue was closed.
Change committed as 202471 |
