Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2471253003:
Page Info (native Mac): truncate permission labels and add tooltip. (Closed)
DescriptionPage Info (native Mac): truncate permission labels and add tooltip.
BUG=654268
TEST=Try the following on Mac twice, once with Chrome set to German, once with it set to Hebrew (change this in `System Preferences.app` > `Language & Region` > Add to `Preferred languages:` and drag to top).
1) Visit google.com and click on the lock icon in the omnibox to open Page Info
2) Verify that the last permission (MIDI, with a keyboard icon) has a label is truncated using an ellipsis (note: Hebrew is an RTL language, so the ellipsis for it should be on the left)
3) Hover over the label, and check that it shows a full version of the truncated string.
Committed: https://crrev.com/4d4e9096b9e299a1d910c2647c6328f30110b0c2
Cr-Commit-Position: refs/heads/master@{#429665}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 21 (8 generated)
lgarron@chromium.org changed reviewers: + rsesek@chromium.org
rsesek@, could you review? (Note: I'm being asked to merge this to M55, so it's a little time-sensitive. Let me know if I should find another reviewer.)
Description was changed from ========== Page Info (native Mac): truncate permission labels and add tooltip. BUG=654268 ========== to ========== Page Info (native Mac): truncate permission labels and add tooltip. BUG=654268 TEST=Try the following on Mac twice, once with Chrome set to German, once with it set to Arabic (change this in `System Preferences.app` > `Language & Region` > Add to `Preferred languages:` and drag to top). 1) Visit google.com and click on the lock icon in the omnibox to open Page Info 2) Verify that the last permission (MIDI, with a keyboard icon) has a label is truncated using an ellipsis (note: Hebrew is an RTL language, so the ellipsis for it should be on the left) 3) Hover over the label, and check that it shows a full version of the truncated string. ==========
https://codereview.chromium.org/2471253003/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/2471253003/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:897: [label setFrame:labelFrame]; These two lines are duplicated within the two branches. Collapse?
https://codereview.chromium.org/2471253003/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/2471253003/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:897: [label setFrame:labelFrame]; On 2016/11/03 at 00:04:31, Robert Sesek wrote: > These two lines are duplicated within the two branches. Collapse? They are conditional within both branches. Do you have a way to share the code without using more lines?
https://codereview.chromium.org/2471253003/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/2471253003/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:897: [label setFrame:labelFrame]; On 2016/11/03 at 00:05:21, lgarron wrote: > On 2016/11/03 at 00:04:31, Robert Sesek wrote: > > These two lines are duplicated within the two branches. Collapse? > > They are conditional within both branches. Do you have a way to share the code without using more lines? Note that putting this inside the conditional inside for RTL is necessary, else the RTL text will be aligned on the wrong side. (I could try to set text direction or alignment inside `label` for RTL instead, but I didn't quickly find the "right" way to do that.)
Oh hm I see. lgtm
The CQ bit was checked by lgarron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by lgarron@chromium.org
lgarron@chromium.org changed reviewers: + shrike@chromium.org
shrike@, could you explicitly review? (See screenshots at https://bugs.chromium.org/p/chromium/issues/detail?id=654268#c27) In the interest of time, I will land this tomorrow if I don't hear from you.
Hello lgarron@, This solution is at least better than what's there now. My only question is what happens with the truncated attribute names with VoiceOver? Are the truncated names fully spoken?
I just verified; VoiceOver speaks the full text.
The CQ bit was checked by lgarron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for confirming.
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Page Info (native Mac): truncate permission labels and add tooltip. BUG=654268 TEST=Try the following on Mac twice, once with Chrome set to German, once with it set to Arabic (change this in `System Preferences.app` > `Language & Region` > Add to `Preferred languages:` and drag to top). 1) Visit google.com and click on the lock icon in the omnibox to open Page Info 2) Verify that the last permission (MIDI, with a keyboard icon) has a label is truncated using an ellipsis (note: Hebrew is an RTL language, so the ellipsis for it should be on the left) 3) Hover over the label, and check that it shows a full version of the truncated string. ========== to ========== Page Info (native Mac): truncate permission labels and add tooltip. BUG=654268 TEST=Try the following on Mac twice, once with Chrome set to German, once with it set to Arabic (change this in `System Preferences.app` > `Language & Region` > Add to `Preferred languages:` and drag to top). 1) Visit google.com and click on the lock icon in the omnibox to open Page Info 2) Verify that the last permission (MIDI, with a keyboard icon) has a label is truncated using an ellipsis (note: Hebrew is an RTL language, so the ellipsis for it should be on the left) 3) Hover over the label, and check that it shows a full version of the truncated string. Committed: https://crrev.com/4d4e9096b9e299a1d910c2647c6328f30110b0c2 Cr-Commit-Position: refs/heads/master@{#429665} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/4d4e9096b9e299a1d910c2647c6328f30110b0c2 Cr-Commit-Position: refs/heads/master@{#429665}
Message was sent while issue was closed.
Description was changed from ========== Page Info (native Mac): truncate permission labels and add tooltip. BUG=654268 TEST=Try the following on Mac twice, once with Chrome set to German, once with it set to Arabic (change this in `System Preferences.app` > `Language & Region` > Add to `Preferred languages:` and drag to top). 1) Visit google.com and click on the lock icon in the omnibox to open Page Info 2) Verify that the last permission (MIDI, with a keyboard icon) has a label is truncated using an ellipsis (note: Hebrew is an RTL language, so the ellipsis for it should be on the left) 3) Hover over the label, and check that it shows a full version of the truncated string. Committed: https://crrev.com/4d4e9096b9e299a1d910c2647c6328f30110b0c2 Cr-Commit-Position: refs/heads/master@{#429665} ========== to ========== Page Info (native Mac): truncate permission labels and add tooltip. BUG=654268 TEST=Try the following on Mac twice, once with Chrome set to German, once with it set to Hebrew (change this in `System Preferences.app` > `Language & Region` > Add to `Preferred languages:` and drag to top). 1) Visit google.com and click on the lock icon in the omnibox to open Page Info 2) Verify that the last permission (MIDI, with a keyboard icon) has a label is truncated using an ellipsis (note: Hebrew is an RTL language, so the ellipsis for it should be on the left) 3) Hover over the label, and check that it shows a full version of the truncated string. Committed: https://crrev.com/4d4e9096b9e299a1d910c2647c6328f30110b0c2 Cr-Commit-Position: refs/heads/master@{#429665} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
