Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
222033002:
Add an 'obscured' flag to views::Label. (Closed)
|
Created:
6 years, 8 months ago by Mike West Modified:
6 years, 8 months ago CC:
chromium-reviews, tfarina, Patrick Dubroy Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd an 'obscured' flag to views::Label.
For the password management bubble, we're rendering passwords into a
label after replacing the string with bullets. This is something that's
available at the platform level on OSX (NSSecureTextField); it seems
reasonable to add the capability to labels themselves, rather than
special-casing it into a view just for this bubble.
'git cl format' produced minor formatting changes in the effected lines.
BUG=328339
R=msw@chromium.org, sadrul@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261728
Patch Set 1 : AppEngine... #Patch Set 2 : *facepalm* #
Total comments: 15
Patch Set 3 : Test. #
Total comments: 6
Patch Set 4 : Feedback #
Messages
Total messages: 27 (0 generated)
sadrul@: Would you mind taking a look at this CL? Does this feature make sense for views::Label (as a quasi-corollary to OSX's NSSecureTextView)? dubroy@: FYI.
On 2014/04/02 10:47:59, Mike West wrote: > sadrul@: Would you mind taking a look at this CL? Does this feature make sense > for views::Label (as a quasi-corollary to OSX's NSSecureTextView)? > > dubroy@: FYI. This looks reasonable, but would a readonly password textfield be more suited for this use case? (it looks like the settings page also does this, and we can change the textfield appearance in views for this if we need it to be displayed differently)
In this particular case, we want to hide the value of a Link. I'm not familiar enough with views to determine whether click behavior is easy/sane to replicate with a text field. I'm happy to do whatever you're happiest with. :) On Apr 2, 2014 6:59 PM, <sadrul@chromium.org> wrote: > On 2014/04/02 10:47:59, Mike West wrote: > >> sadrul@: Would you mind taking a look at this CL? Does this feature make >> sense >> for views::Label (as a quasi-corollary to OSX's NSSecureTextView)? >> > > dubroy@: FYI. >> > > This looks reasonable, but would a readonly password textfield be more > suited > for this use case? (it looks like the settings page also does this, and we > can > change the textfield appearance in views for this if we need it to be > displayed > differently) > > https://codereview.chromium.org/222033002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
+msw@ for thoughts as textfield owner On 2014/04/02 17:05:35, Use mkwst_at_chromium.org plz. wrote: > In this particular case, we want to hide the value of a Link. I'm not > familiar enough with views to determine whether click behavior is easy/sane > to replicate with a text field. It should be possible to install a ui::EventHandler on the Textfield as a pre-target handler (textfield->AddPreTargetHandler(handler);), where the handler will override EventHandler::OnMouseEvent() and process the mouse-press/release event to do something interesting (e.g. make the textfield editable again?). > > I'm happy to do whatever you're happiest with. :) > On Apr 2, 2014 6:59 PM, <mailto:sadrul@chromium.org> wrote: > > > On 2014/04/02 10:47:59, Mike West wrote: > > > >> sadrul@: Would you mind taking a look at this CL? Does this feature make > >> sense > >> for views::Label (as a quasi-corollary to OSX's NSSecureTextView)? > >> > > > > dubroy@: FYI. > >> > > > > This looks reasonable, but would a readonly password textfield be more > > suited > > for this use case? (it looks like the settings page also does this, and we > > can > > change the textfield appearance in views for this if we need it to be > > displayed > > differently) > > > > https://codereview.chromium.org/222033002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
I'm not opposed to this in principal, but sad that I still haven't backed views::Label with a gfx::RenderText instance (like views::Textfield), which would give us this functionality essentially for free. This is a large cleanup/performance task that I have started, but not found time to finish: https://codereview.chromium.org/23228004. If you're inclined, using a read-only views::Textfield would avoid code duplication, and keep consistency in the display of obscured text, but you'd need a subclass that overrides OnMouse[Pressed|Released] like views::Link to supply the link-clicking functionality, and you'd need to apply the link color[s] and underline. https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.... ui/views/controls/label.cc:69: is_obscured_ ? base::string16(text_.length(), 0x2022) : base::string16(); This should match obscured textfields, specifically the |obscured_text_length| calculation in RenderText::UpdateLayoutText() (which renders a surrogate pair of characters as a single asterisk, I think): gfx::UTF16IndexToOffset(text_, 0, text_.length()) It would also make sense to use the same replacement character as textfields, kPasswordReplacementChar = '*'. (if you prefer 0x2022, we can ask UX about using that for both, but I highly encourage the two being consistent) https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.... ui/views/controls/label.cc:151: obscured_text_ = is_obscured_ ? base::string16(text_.length(), 0x2022) Call SetText() here (by changing the initial text == text_ check to also check for the expected obscured_text_.length()), or make a private SetTextInternal that avoids the |text == text_| check and call that, or make some other helper that consolidates the duplicate code. https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.... ui/views/controls/label.h:140: // Get the label text as displayed to the user, respecting the 'obscured' nit: drop the word "label" for a one-line comment. https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.... ui/views/controls/label.h:142: const base::string16& display_text() const; nit: non-inlined functions should have CamelCaseNaming; use GetLayoutText(). https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.... ui/views/controls/label.h:251: base::string16 obscured_text_; nit: Call this |layout_text| to be consistent with RenderText. https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label_... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label_... ui/views/controls/label_unittest.cc:108: EXPECT_EQ(ASCIIToUTF16("Password!"), label.display_text()); nit: re-use test_text here and below. https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label_... ui/views/controls/label_unittest.cc:115: nit: test that a SetText call while the Label is obscured works as expected. https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label_... ui/views/controls/label_unittest.cc:159: label.SetObscured(false); nit: after this call, EXPECT_TRUE(label.GetTooltipText(gfx::Point(), &tooltip)); (ensure that the obscured flag doesn't permanently clobber the tooltip)
> I'm not opposed to this in principal, but sad that I still haven't backed > views::Label with a gfx::RenderText instance (like views::Textfield), which > would give us this functionality essentially for free. This is a large > cleanup/performance task that I have started, but not found time to finish: > https://codereview.chromium.org/23228004. This patch wouldn't seem to exclude that work; wiring up the flag to the RenderText concept obscured text looks trivial (once the hard work of migrating onto that backend is done, of course :) ). > If you're inclined, using a read-only views::Textfield would avoid code > duplication, and keep consistency in the display of obscured text, but you'd > need a subclass that overrides OnMouse[Pressed|Released] like views::Link to > supply the link-clicking functionality, and you'd need to apply the link > color[s] and underline. Given that we're talking about ~10 lines of code in the framework, I would prefer to add this to Label/Link directly; I suspect it would be quite a bit more duplication (albeit in my code rather than the framework) to get the Link functionality on top of Textfield. I'm happy to go that route if you have a strong preference, of course. But I've updated the patch as you suggested in the hopes that it's acceptable. Thanks for the excellent review; your comments were quite helpful. https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.... ui/views/controls/label.cc:69: is_obscured_ ? base::string16(text_.length(), 0x2022) : base::string16(); On 2014/04/03 01:38:48, msw wrote: > This should match obscured textfields, specifically the |obscured_text_length| > calculation in RenderText::UpdateLayoutText() (which renders a surrogate pair of > characters as a single asterisk, I think): gfx::UTF16IndexToOffset(text_, 0, > text_.length()) > > It would also make sense to use the same replacement character as textfields, > kPasswordReplacementChar = '*'. (if you prefer 0x2022, we can ask UX about using > that for both, but I highly encourage the two being consistent) This was very helpful, thank you. I've pulled in the logic from UpdateLayoutText as you suggested, and added a surrogate pair test to verify that we're only producing one '*' for such a character representation. I've also switched to using '*' rather than 0x2022; I'll ping UX folks about that, as they'd suggested 0x2022 specifically. If there's good reason to use that bullet rather than an asterisk, I'll change RenderText in a future patch. https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.... ui/views/controls/label.cc:151: obscured_text_ = is_obscured_ ? base::string16(text_.length(), 0x2022) On 2014/04/03 01:38:48, msw wrote: > Call SetText() here (by changing the initial text == text_ check to also check > for the expected obscured_text_.length()), or make a private SetTextInternal > that avoids the |text == text_| check and call that, or make some other helper > that consolidates the duplicate code. Good idea, done. https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.... ui/views/controls/label.h:140: // Get the label text as displayed to the user, respecting the 'obscured' On 2014/04/03 01:38:48, msw wrote: > nit: drop the word "label" for a one-line comment. Done. https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.... ui/views/controls/label.h:142: const base::string16& display_text() const; On 2014/04/03 01:38:48, msw wrote: > nit: non-inlined functions should have CamelCaseNaming; use GetLayoutText(). Done. https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label.... ui/views/controls/label.h:251: base::string16 obscured_text_; On 2014/04/03 01:38:48, msw wrote: > nit: Call this |layout_text| to be consistent with RenderText. Done.
LGTM with nits; agreed that this seems reasonable. Yeah, this doesn't hinder Label using RenderText. https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label_... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label_... ui/views/controls/label_unittest.cc:159: label.SetObscured(false); On 2014/04/03 01:38:48, msw wrote: > nit: after this call, EXPECT_TRUE(label.GetTooltipText(gfx::Point(), &tooltip)); > (ensure that the obscured flag doesn't permanently clobber the tooltip) ping. https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:65: if (text == text_) nit: flip the condition and drop the return statement. https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label... ui/views/controls/label_unittest.cc:108: EXPECT_EQ(test_text, label.layout_text()); nit: test the value of text() here and elsewhere. https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label... ui/views/controls/label_unittest.cc:127: label.SetText(test_text + test_text); nit: this could be part of the test above.
On 2014/04/03 18:27:48, msw wrote: > LGTM with nits; agreed that this seems reasonable. > Yeah, this doesn't hinder Label using RenderText. > > https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label_... > File ui/views/controls/label_unittest.cc (right): > > https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label_... > ui/views/controls/label_unittest.cc:159: label.SetObscured(false); > On 2014/04/03 01:38:48, msw wrote: > > nit: after this call, EXPECT_TRUE(label.GetTooltipText(gfx::Point(), > &tooltip)); > > (ensure that the obscured flag doesn't permanently clobber the tooltip) > > ping. > > https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label.cc > File ui/views/controls/label.cc (right): > > https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label... > ui/views/controls/label.cc:65: if (text == text_) > nit: flip the condition and drop the return statement. > > https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label... > File ui/views/controls/label_unittest.cc (right): > > https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label... > ui/views/controls/label_unittest.cc:108: EXPECT_EQ(test_text, > label.layout_text()); > nit: test the value of text() here and elsewhere. > > https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label... > ui/views/controls/label_unittest.cc:127: label.SetText(test_text + test_text); > nit: this could be part of the test above. I'll take care of these tomorrow, thanks again for the review. sadrul@: If you agree, would you mind stamping the patch so I can land it tomorrow morning in Europe? :)
stamp lgtm
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkwst@chromium.org/222033002/120001
Thanks! https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label_... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/222033002/diff/40001/ui/views/controls/label_... ui/views/controls/label_unittest.cc:159: label.SetObscured(false); On 2014/04/03 18:27:48, msw wrote: > On 2014/04/03 01:38:48, msw wrote: > > nit: after this call, EXPECT_TRUE(label.GetTooltipText(gfx::Point(), > &tooltip)); > > (ensure that the obscured flag doesn't permanently clobber the tooltip) > > ping. Done. https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:65: if (text == text_) On 2014/04/03 18:27:48, msw wrote: > nit: flip the condition and drop the return statement. Done. https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label... ui/views/controls/label_unittest.cc:108: EXPECT_EQ(test_text, label.layout_text()); On 2014/04/03 18:27:48, msw wrote: > nit: test the value of text() here and elsewhere. Done. https://codereview.chromium.org/222033002/diff/100001/ui/views/controls/label... ui/views/controls/label_unittest.cc:127: label.SetText(test_text + test_text); On 2014/04/03 18:27:48, msw wrote: > nit: this could be part of the test above. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkwst@chromium.org/222033002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkwst@chromium.org/222033002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkwst@chromium.org/222033002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
Message was sent while issue was closed.
Committed patchset #4 manually as r261728 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
