New issue
Advanced search Search tips

Issue 775496 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[views::Combobox] OnComboboxModelChanged should trigger re-painting

Project Member Reported by kolos@chromium.org, Oct 17 2017

Issue description

At this point, Combobox::ModelChanged doesn't trigger any repainting.

The model is a list of items in the combobox. Once the list changed, the combobox should be re-painted, shouldn't it?

msw@: please explain whether I am wrong or not. Thx.
 

Comment 1 by msw@chromium.org, Oct 17 2017

Cc: msw@chromium.org
Owner: kolos@chromium.org
I don't think we support changing combobox contents while they're open.
Do you have some use case where this happens or matters?
I don't see how it's related to  issue 771878 .

This isn't something I'd work on, nor should we implement without a good reason.
If we did change contents while open, how does that affect user interaction?
(ie. if you're about to click an entry and they shift/change, that's frustrating)
I would resolve this WontFix, but maybe you care to explain a bit more.

Comment 2 by kolos@chromium.org, Oct 17 2017

We need it for issue 753806. When a user click on the eye icon, the passwords in the combobox become masked/revealed. The combobox may be closed, but the selected item changes appearance. Does it make sense? 

I have call repaint here https://chromium-review.googlesource.com/c/chromium/src/+/723319/5/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc#597 

Comment 3 by msw@chromium.org, Oct 17 2017

Ah, you mean what's shown when the combobox is closed; sorry for misunderstanding.
Yeah, I suppose Combobox::ModelChanged() could call SchedulePaint() internally.

Comment 4 by kolos@chromium.org, Oct 17 2017

Will you have a chance to fix it?

Comment 5 by msw@chromium.org, Oct 17 2017

This seems simple enough to include in your existing patch set, I'll review.

Comment 6 by kolos@chromium.org, Oct 18 2017

I am going to merge https://chromium-review.googlesource.com/c/chromium/src/+/723319 to M-63. To keep things safer, I will do in a separate CL that affects on all comboboxes.

Comment 7 by kolos@chromium.org, Oct 19 2017

Status: Started (was: Assigned)
Project Member

Comment 8 by bugdroid1@chromium.org, Nov 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/053f61ae949484a98b834541ffd51307ca678713

commit 053f61ae949484a98b834541ffd51307ca678713
Author: Maxim Kolosovskiy <kolos@chromium.org>
Date: Fri Nov 03 16:39:36 2017

[Views][Combobox] Schedule re-paint once a combobox model has changed

Bug:  775496 
Change-Id: I0a1a6884f57cfa3a587a0b02da6cadb437a03fb8
Reviewed-on: https://chromium-review.googlesource.com/727886
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513801}
[modify] https://crrev.com/053f61ae949484a98b834541ffd51307ca678713/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
[modify] https://crrev.com/053f61ae949484a98b834541ffd51307ca678713/ui/views/controls/combobox/combobox.cc

Comment 9 by kolos@chromium.org, Nov 3 2017

Status: Fixed (was: Started)

Sign in to add a comment