Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
484133002:
"F4" doesn't close/open the "Folder" dropdown on "Bookmark added!" pop-up. (Closed)
Description"F4" doesn't close/open the "Folder" dropdown on "Bookmark added!" pop-up.
When F4 is selected and the submenu is showing then we should hide
submenu instead of showing the submenu item selection or opening submenu.
Condition added to check this situation.
BUG=344491
Committed: https://crrev.com/90a0b2153dfde826d9e351ce2f74c59119ca474a
Cr-Commit-Position: refs/heads/master@{#294096}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Change comment as per reviewers comment. #Patch Set 3 : modifying comments. #Messages
Total messages: 27 (2 generated)
PTAL
Nice work, the code itself looks okay (with a comment nit). Hopefully Mark knows if you need to sign a CLA or similar. https://codereview.chromium.org/484133002/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/484133002/diff/1/AUTHORS#newcode99 AUTHORS:99: Deepak Mittal <deepak.m1@samsung.com> Mark, does Deepak need to sign a CLA (or amend a corp one)? The instructions at < http://www.chromium.org/developers/contributing-code/external-contributor-che... > aren't entirely obvious for this case. I don't see other explicit entries in http://goto/cla-signers for @samsung addresses, but the instructions say: "If there's a corporate CLA for the author's company, it must list the person explicitly (or the list of authorized contributors must say something like "All employees"). If the author is not on their company's roster, do not accept the change." (where do I find Samsung's roster?) https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_controller.cc:1037: // Fallthrough to accept on F4, so combobox menus match Windows behavior. nit: "Fallthrough to accept or dismiss combobox menus on F4, like Windows."
deepak.m1@samsung.com is listed as an authorized contributor in Samsung's list (on the Corporate Signers tab). So, looks fine to me. On Tue, Aug 19, 2014 at 6:05 PM, <msw@chromium.org> wrote: > Nice work, the code itself looks okay (with a comment nit). > Hopefully Mark knows if you need to sign a CLA or similar. > > > https://codereview.chromium.org/484133002/diff/1/AUTHORS > File AUTHORS (right): > > https://codereview.chromium.org/484133002/diff/1/AUTHORS#newcode99 > AUTHORS:99: Deepak Mittal <deepak.m1@samsung.com> > Mark, does Deepak need to sign a CLA (or amend a corp one)? > > The instructions at < > http://www.chromium.org/developers/contributing-code/ > external-contributor-checklist > >> aren't entirely obvious for this case. I don't see other explicit >> > entries in http://goto/cla-signers for @samsung addresses, but the > instructions say: "If there's a corporate CLA for the author's company, > it must list the person explicitly (or the list of authorized > contributors must say something like "All employees"). If the author is > not on their company's roster, do not accept the change." (where do I > find Samsung's roster?) > > https://codereview.chromium.org/484133002/diff/1/ui/views/ > controls/menu/menu_controller.cc > File ui/views/controls/menu/menu_controller.cc (right): > > https://codereview.chromium.org/484133002/diff/1/ui/views/ > controls/menu/menu_controller.cc#newcode1037 > ui/views/controls/menu/menu_controller.cc:1037: // Fallthrough to accept > on F4, so combobox menus match Windows behavior. > nit: "Fallthrough to accept or dismiss combobox menus on F4, like > Windows." > > https://codereview.chromium.org/484133002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Great! Then lgtm with a comment nit. > mailto:deepak.m1@samsung.com is listed as an authorized contributor in Samsung's > list (on the Corporate Signers tab). So, looks fine to me. > > https://codereview.chromium.org/484133002/diff/1/ui/views/ > > controls/menu/menu_controller.cc#newcode1037 > > ui/views/controls/menu/menu_controller.cc:1037: // Fallthrough to accept > > on F4, so combobox menus match Windows behavior. > > nit: "Fallthrough to accept or dismiss combobox menus on F4, like > > Windows."
https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_controller.cc:1042: pending_state_.item->GetSubmenu()->IsShowing()) Why does it matter whether the submenu is showing?
https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_controller.cc:1037: // Fallthrough to accept on F4, so combobox menus match Windows behavior. On 2014/08/20 01:05:01, msw wrote: > nit: "Fallthrough to accept or dismiss combobox menus on F4, like Windows." Done. https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_controller.cc:1042: pending_state_.item->GetSubmenu()->IsShowing()) On 2014/08/20 02:45:39, sky wrote: > Why does it matter whether the submenu is showing? As When Submneu is shown, and we select F4, we wanted to dismiss submenu and don't want to show the selection in the submenu. and if submenu is not shown and we select F4 then we want submenu to be shown, To get the same windows behavior.
https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... ui/views/controls/menu/menu_controller.cc:1042: pending_state_.item->GetSubmenu()->IsShowing()) On 2014/08/20 03:37:33, deepak.m1 wrote: > On 2014/08/20 02:45:39, sky wrote: > > Why does it matter whether the submenu is showing? > As When Submneu is shown, and we select F4, we wanted to dismiss submenu and > don't want to show the selection in the submenu. > > and if submenu is not shown and we select F4 then we want submenu to be shown, > To get the same windows behavior. Comboboxs don't have submenus, right?
On 2014/08/20 15:21:39, sky wrote: > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > File ui/views/controls/menu/menu_controller.cc (right): > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > ui/views/controls/menu/menu_controller.cc:1042: > pending_state_.item->GetSubmenu()->IsShowing()) > On 2014/08/20 03:37:33, deepak.m1 wrote: > > On 2014/08/20 02:45:39, sky wrote: > > > Why does it matter whether the submenu is showing? > > As When Submneu is shown, and we select F4, we wanted to dismiss submenu and > > don't want to show the selection in the submenu. > > > > and if submenu is not shown and we select F4 then we want submenu to be shown, > > To get the same windows behavior. > > Comboboxs don't have submenus, right? By Submenu, my mean is the dropdown that is coming while selecting the combobox. When combobox is focused, and user select First 'F4' then dropdown from the combobox came that is submenu. and then on selecting 'F4' second time, controls comes in if (pending_state_.item->HasSubmenu()) { OpenSubmenuChangeSelectionIfCan(); } here pending_state_.item type is also SUBMENU ,so above 'if' condition become true and it will call OpenSubmenuChangeSelectionIfCan() , here it will put selection at the first item in the dropdown list by: SetSelection(item->GetSubmenu()->GetMenuItemAt(0), SELECTION_UPDATE_IMMEDIATELY); And when we select the 'F4' third time then pending_state_.item type is NORMAL and then it will go in the Accept() and set EXIT_ALL, so submenu hided. so I have put the check (on second 'F4' selection) that when pending_state_.item is submenu present and it is showing then we need to 'return false' that will close the submenu as 'should_quit' value become true.so combobox menu will matche windows behavior.
On 2014/08/21 04:59:32, deepak.m1 wrote: > On 2014/08/20 15:21:39, sky wrote: > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > File ui/views/controls/menu/menu_controller.cc (right): > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > ui/views/controls/menu/menu_controller.cc:1042: > > pending_state_.item->GetSubmenu()->IsShowing()) > > On 2014/08/20 03:37:33, deepak.m1 wrote: > > > On 2014/08/20 02:45:39, sky wrote: > > > > Why does it matter whether the submenu is showing? > > > As When Submneu is shown, and we select F4, we wanted to dismiss submenu and > > > don't want to show the selection in the submenu. > > > > > > and if submenu is not shown and we select F4 then we want submenu to be > shown, > > > To get the same windows behavior. > > > > Comboboxs don't have submenus, right? > > By Submenu, my mean is the dropdown that is coming while selecting the combobox. > When combobox is focused, and user select First 'F4' then dropdown from the > combobox came that is submenu. > and then on selecting 'F4' second time, controls comes in > if (pending_state_.item->HasSubmenu()) { > OpenSubmenuChangeSelectionIfCan(); > } > here pending_state_.item type is also SUBMENU ,so above 'if' condition become > true and it will call OpenSubmenuChangeSelectionIfCan() , here it will put > selection at the first item in the dropdown list by: > SetSelection(item->GetSubmenu()->GetMenuItemAt(0), > SELECTION_UPDATE_IMMEDIATELY); > > And when we select the 'F4' third time then pending_state_.item type is NORMAL > and then it will go in the Accept() and set EXIT_ALL, so submenu hided. > > so I have put the check (on second 'F4' selection) that when pending_state_.item > is submenu present and it is showing then we need to 'return false' that will > close the submenu as 'should_quit' value become true.so combobox menu will > matche windows behavior. Please have a look at my analysis in comment #8 and let me know your opinion. So that we can conclude on this.Thanks for your time.
On 2014/08/26 12:00:48, deepak.m1 wrote: > On 2014/08/21 04:59:32, deepak.m1 wrote: > > On 2014/08/20 15:21:39, sky wrote: > > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > > File ui/views/controls/menu/menu_controller.cc (right): > > > > > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > > ui/views/controls/menu/menu_controller.cc:1042: > > > pending_state_.item->GetSubmenu()->IsShowing()) > > > On 2014/08/20 03:37:33, deepak.m1 wrote: > > > > On 2014/08/20 02:45:39, sky wrote: > > > > > Why does it matter whether the submenu is showing? > > > > As When Submneu is shown, and we select F4, we wanted to dismiss submenu > and > > > > don't want to show the selection in the submenu. > > > > > > > > and if submenu is not shown and we select F4 then we want submenu to be > > shown, > > > > To get the same windows behavior. > > > > > > Comboboxs don't have submenus, right? > > > > By Submenu, my mean is the dropdown that is coming while selecting the > combobox. > > When combobox is focused, and user select First 'F4' then dropdown from the > > combobox came that is submenu. > > and then on selecting 'F4' second time, controls comes in > > if (pending_state_.item->HasSubmenu()) { > > OpenSubmenuChangeSelectionIfCan(); > > } > > here pending_state_.item type is also SUBMENU ,so above 'if' condition become > > true and it will call OpenSubmenuChangeSelectionIfCan() , here it will put > > selection at the first item in the dropdown list by: > > SetSelection(item->GetSubmenu()->GetMenuItemAt(0), > > SELECTION_UPDATE_IMMEDIATELY); > > > > And when we select the 'F4' third time then pending_state_.item type is NORMAL > > and then it will go in the Accept() and set EXIT_ALL, so submenu hided. > > > > so I have put the check (on second 'F4' selection) that when > pending_state_.item > > is submenu present and it is showing then we need to 'return false' that will > > close the submenu as 'should_quit' value become true.so combobox menu will > > matche windows behavior. > > Please have a look at my analysis in comment #8 and let me know your opinion. > So that we can conclude on this.Thanks for your time. @sky PTAL
On 2014/08/26 12:00:48, deepak.m1 wrote: > On 2014/08/21 04:59:32, deepak.m1 wrote: > > On 2014/08/20 15:21:39, sky wrote: > > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > > File ui/views/controls/menu/menu_controller.cc (right): > > > > > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > > ui/views/controls/menu/menu_controller.cc:1042: > > > pending_state_.item->GetSubmenu()->IsShowing()) > > > On 2014/08/20 03:37:33, deepak.m1 wrote: > > > > On 2014/08/20 02:45:39, sky wrote: > > > > > Why does it matter whether the submenu is showing? > > > > As When Submneu is shown, and we select F4, we wanted to dismiss submenu > and > > > > don't want to show the selection in the submenu. > > > > > > > > and if submenu is not shown and we select F4 then we want submenu to be > > shown, > > > > To get the same windows behavior. > > > > > > Comboboxs don't have submenus, right? > > > > By Submenu, my mean is the dropdown that is coming while selecting the > combobox. > > When combobox is focused, and user select First 'F4' then dropdown from the > > combobox came that is submenu. > > and then on selecting 'F4' second time, controls comes in > > if (pending_state_.item->HasSubmenu()) { > > OpenSubmenuChangeSelectionIfCan(); > > } > > here pending_state_.item type is also SUBMENU ,so above 'if' condition become > > true and it will call OpenSubmenuChangeSelectionIfCan() , here it will put > > selection at the first item in the dropdown list by: > > SetSelection(item->GetSubmenu()->GetMenuItemAt(0), > > SELECTION_UPDATE_IMMEDIATELY); > > > > And when we select the 'F4' third time then pending_state_.item type is NORMAL > > and then it will go in the Accept() and set EXIT_ALL, so submenu hided. > > > > so I have put the check (on second 'F4' selection) that when > pending_state_.item > > is submenu present and it is showing then we need to 'return false' that will > > close the submenu as 'should_quit' value become true.so combobox menu will > > matche windows behavior. > > Please have a look at my analysis in comment #8 and let me know your opinion. > So that we can conclude on this.Thanks for your time. F4 is only used for comboboxs, right?
On 2014/08/28 16:26:12, sky wrote: > On 2014/08/26 12:00:48, deepak.m1 wrote: > > On 2014/08/21 04:59:32, deepak.m1 wrote: > > > On 2014/08/20 15:21:39, sky wrote: > > > > > > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > > > File ui/views/controls/menu/menu_controller.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > > > ui/views/controls/menu/menu_controller.cc:1042: > > > > pending_state_.item->GetSubmenu()->IsShowing()) > > > > On 2014/08/20 03:37:33, deepak.m1 wrote: > > > > > On 2014/08/20 02:45:39, sky wrote: > > > > > > Why does it matter whether the submenu is showing? > > > > > As When Submneu is shown, and we select F4, we wanted to dismiss submenu > > and > > > > > don't want to show the selection in the submenu. > > > > > > > > > > and if submenu is not shown and we select F4 then we want submenu to be > > > shown, > > > > > To get the same windows behavior. > > > > > > > > Comboboxs don't have submenus, right? > > > > > > By Submenu, my mean is the dropdown that is coming while selecting the > > combobox. > > > When combobox is focused, and user select First 'F4' then dropdown from the > > > combobox came that is submenu. > > > and then on selecting 'F4' second time, controls comes in > > > if (pending_state_.item->HasSubmenu()) { > > > OpenSubmenuChangeSelectionIfCan(); > > > } > > > here pending_state_.item type is also SUBMENU ,so above 'if' condition > become > > > true and it will call OpenSubmenuChangeSelectionIfCan() , here it will put > > > selection at the first item in the dropdown list by: > > > SetSelection(item->GetSubmenu()->GetMenuItemAt(0), > > > SELECTION_UPDATE_IMMEDIATELY); > > > > > > And when we select the 'F4' third time then pending_state_.item type is > NORMAL > > > and then it will go in the Accept() and set EXIT_ALL, so submenu hided. > > > > > > so I have put the check (on second 'F4' selection) that when > > pending_state_.item > > > is submenu present and it is showing then we need to 'return false' that > will > > > close the submenu as 'should_quit' value become true.so combobox menu will > > > matche windows behavior. > > > > Please have a look at my analysis in comment #8 and let me know your opinion. > > So that we can conclude on this.Thanks for your time. > > F4 is only used for comboboxs, right? In the issue scenerio, F4 is getting used after focus comes to combobox to open that and then doing selection to it. But after my chnages F4 will be used to open and closing combobox. I request , please refer to my explanation in comment #8.
On 2014/08/29 09:59:30, deepak.m1 wrote: > On 2014/08/28 16:26:12, sky wrote: > > On 2014/08/26 12:00:48, deepak.m1 wrote: > > > On 2014/08/21 04:59:32, deepak.m1 wrote: > > > > On 2014/08/20 15:21:39, sky wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > > > > File ui/views/controls/menu/menu_controller.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > > > > ui/views/controls/menu/menu_controller.cc:1042: > > > > > pending_state_.item->GetSubmenu()->IsShowing()) > > > > > On 2014/08/20 03:37:33, deepak.m1 wrote: > > > > > > On 2014/08/20 02:45:39, sky wrote: > > > > > > > Why does it matter whether the submenu is showing? > > > > > > As When Submneu is shown, and we select F4, we wanted to dismiss > submenu > > > and > > > > > > don't want to show the selection in the submenu. > > > > > > > > > > > > and if submenu is not shown and we select F4 then we want submenu to > be > > > > shown, > > > > > > To get the same windows behavior. > > > > > > > > > > Comboboxs don't have submenus, right? > > > > > > > > By Submenu, my mean is the dropdown that is coming while selecting the > > > combobox. > > > > When combobox is focused, and user select First 'F4' then dropdown from > the > > > > combobox came that is submenu. > > > > and then on selecting 'F4' second time, controls comes in > > > > if (pending_state_.item->HasSubmenu()) { > > > > OpenSubmenuChangeSelectionIfCan(); > > > > } > > > > here pending_state_.item type is also SUBMENU ,so above 'if' condition > > become > > > > true and it will call OpenSubmenuChangeSelectionIfCan() , here it will put > > > > selection at the first item in the dropdown list by: > > > > SetSelection(item->GetSubmenu()->GetMenuItemAt(0), > > > > SELECTION_UPDATE_IMMEDIATELY); > > > > > > > > And when we select the 'F4' third time then pending_state_.item type is > > NORMAL > > > > and then it will go in the Accept() and set EXIT_ALL, so submenu hided. > > > > > > > > so I have put the check (on second 'F4' selection) that when > > > pending_state_.item > > > > is submenu present and it is showing then we need to 'return false' that > > will > > > > close the submenu as 'should_quit' value become true.so combobox menu will > > > > matche windows behavior. > > > > > > Please have a look at my analysis in comment #8 and let me know your > opinion. > > > So that we can conclude on this.Thanks for your time. > > > > F4 is only used for comboboxs, right? > In the issue scenerio, F4 is getting used after focus comes to combobox to open that and then second F4 do selection to it. But after my chnages F4 will be used to open and closing combobox. I request ,please refer to my explanation in comment #8. We want same as windows behaviour , so that First F4 will open combobox and second F4 should close combobox.
On 2014/08/29 12:51:58, deepak.m1 wrote: > On 2014/08/29 09:59:30, deepak.m1 wrote: > > On 2014/08/28 16:26:12, sky wrote: > > > On 2014/08/26 12:00:48, deepak.m1 wrote: > > > > On 2014/08/21 04:59:32, deepak.m1 wrote: > > > > > On 2014/08/20 15:21:39, sky wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > > > > > File ui/views/controls/menu/menu_controller.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > > > > > ui/views/controls/menu/menu_controller.cc:1042: > > > > > > pending_state_.item->GetSubmenu()->IsShowing()) > > > > > > On 2014/08/20 03:37:33, deepak.m1 wrote: > > > > > > > On 2014/08/20 02:45:39, sky wrote: > > > > > > > > Why does it matter whether the submenu is showing? > > > > > > > As When Submneu is shown, and we select F4, we wanted to dismiss > > submenu > > > > and > > > > > > > don't want to show the selection in the submenu. > > > > > > > > > > > > > > and if submenu is not shown and we select F4 then we want submenu to > > be > > > > > shown, > > > > > > > To get the same windows behavior. > > > > > > > > > > > > Comboboxs don't have submenus, right? > > > > > > > > > > By Submenu, my mean is the dropdown that is coming while selecting the > > > > combobox. > > > > > When combobox is focused, and user select First 'F4' then dropdown from > > the > > > > > combobox came that is submenu. > > > > > and then on selecting 'F4' second time, controls comes in > > > > > if (pending_state_.item->HasSubmenu()) { > > > > > OpenSubmenuChangeSelectionIfCan(); > > > > > } > > > > > here pending_state_.item type is also SUBMENU ,so above 'if' condition > > > become > > > > > true and it will call OpenSubmenuChangeSelectionIfCan() , here it will > put > > > > > selection at the first item in the dropdown list by: > > > > > SetSelection(item->GetSubmenu()->GetMenuItemAt(0), > > > > > SELECTION_UPDATE_IMMEDIATELY); > > > > > > > > > > And when we select the 'F4' third time then pending_state_.item type is > > > NORMAL > > > > > and then it will go in the Accept() and set EXIT_ALL, so submenu hided. > > > > > > > > > > so I have put the check (on second 'F4' selection) that when > > > > pending_state_.item > > > > > is submenu present and it is showing then we need to 'return false' that > > > will > > > > > close the submenu as 'should_quit' value become true.so combobox menu > will > > > > > matche windows behavior. > > > > > > > > Please have a look at my analysis in comment #8 and let me know your > > opinion. > > > > So that we can conclude on this.Thanks for your time. > > > > > > F4 is only used for comboboxs, right? > > In the issue scenerio, F4 is getting used after focus comes to combobox to open that and then second F4 do selection to it. But after my chnages F4 will be used to open and closing combobox. I request ,please refer to my explanation in comment #8. We want same as windows behaviour , so that First F4 will open combobox and second F4 should close combobox.
On 2014/09/03 06:11:35, deepak.m1 wrote: > On 2014/08/29 12:51:58, deepak.m1 wrote: > > On 2014/08/29 09:59:30, deepak.m1 wrote: > > > On 2014/08/28 16:26:12, sky wrote: > > > > On 2014/08/26 12:00:48, deepak.m1 wrote: > > > > > On 2014/08/21 04:59:32, deepak.m1 wrote: > > > > > > On 2014/08/20 15:21:39, sky wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > > > > > > File ui/views/controls/menu/menu_controller.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/484133002/diff/1/ui/views/controls/menu/menu_... > > > > > > > ui/views/controls/menu/menu_controller.cc:1042: > > > > > > > pending_state_.item->GetSubmenu()->IsShowing()) > > > > > > > On 2014/08/20 03:37:33, deepak.m1 wrote: > > > > > > > > On 2014/08/20 02:45:39, sky wrote: > > > > > > > > > Why does it matter whether the submenu is showing? > > > > > > > > As When Submneu is shown, and we select F4, we wanted to dismiss > > > submenu > > > > > and > > > > > > > > don't want to show the selection in the submenu. > > > > > > > > > > > > > > > > and if submenu is not shown and we select F4 then we want submenu > to > > > be > > > > > > shown, > > > > > > > > To get the same windows behavior. > > > > > > > > > > > > > > Comboboxs don't have submenus, right? > > > > > > > > > > > > By Submenu, my mean is the dropdown that is coming while selecting the > > > > > combobox. > > > > > > When combobox is focused, and user select First 'F4' then dropdown > from > > > the > > > > > > combobox came that is submenu. > > > > > > and then on selecting 'F4' second time, controls comes in > > > > > > if (pending_state_.item->HasSubmenu()) { > > > > > > OpenSubmenuChangeSelectionIfCan(); > > > > > > } > > > > > > here pending_state_.item type is also SUBMENU ,so above 'if' condition > > > > become > > > > > > true and it will call OpenSubmenuChangeSelectionIfCan() , here it will > > put > > > > > > selection at the first item in the dropdown list by: > > > > > > SetSelection(item->GetSubmenu()->GetMenuItemAt(0), > > > > > > SELECTION_UPDATE_IMMEDIATELY); > > > > > > > > > > > > And when we select the 'F4' third time then pending_state_.item type > is > > > > NORMAL > > > > > > and then it will go in the Accept() and set EXIT_ALL, so submenu > hided. > > > > > > > > > > > > so I have put the check (on second 'F4' selection) that when > > > > > pending_state_.item > > > > > > is submenu present and it is showing then we need to 'return false' > that > > > > will > > > > > > close the submenu as 'should_quit' value become true.so combobox menu > > will > > > > > > matche windows behavior. > > > > > > > > > > Please have a look at my analysis in comment #8 and let me know your > > > opinion. > > > > > So that we can conclude on this.Thanks for your time. > > > > > > > > F4 is only used for comboboxs, right? > > > > In the issue scenerio, F4 is getting used after focus comes to combobox to open > that and then second F4 do selection to it. > But after my chnages F4 will be used to open and closing combobox. I request > ,please refer to my explanation in comment #8. > > We want same as windows behaviour , so that First F4 will open combobox and > second F4 should close combobox. @sky PTAL
Did you sign the Google Individual Contributor License Agreement : https://developers.google.com/open-source/cla/individual?csw=1 ?
On 2014/09/04 17:43:01, sky wrote: > Did you sign the Google Individual Contributor License Agreement : > https://developers.google.com/open-source/cla/individual?csw=1 ? yes sky, I have already signed that, as in the #2 review comment. "deepak.m1@samsung.com is listed as an authorized contributor in Samsung's list (on the Corporate Signers tab). So, looks fine to me."
On 2014/09/05 02:55:59, deepak.m1 wrote: > On 2014/09/04 17:43:01, sky wrote: > > Did you sign the Google Individual Contributor License Agreement : > > https://developers.google.com/open-source/cla/individual?csw=1 ? > yes sky, I have already signed that, as in the #3 review comment. mailto:"deepak.m1@samsung.com is listed as an authorized contributor in Samsung's list (on the Corporate Signers tab). So, looks fine to me."
On 2014/09/05 06:24:55, deepak.m1 wrote: > On 2014/09/05 02:55:59, deepak.m1 wrote: > > On 2014/09/04 17:43:01, sky wrote: > > > Did you sign the Google Individual Contributor License Agreement : > > > https://developers.google.com/open-source/cla/individual?csw=1 ? > > yes sky, I have already signed that as part of Samsung Company, as in the #3 review comment. mailto:"deepak.m1@samsung.com is listed as an authorized contributor in Samsung's list (on the Corporate Signers tab). So, looks fine to me."
On 2014/09/06 07:22:40, deepak.m1 wrote: > On 2014/09/05 06:24:55, deepak.m1 wrote: > > On 2014/09/05 02:55:59, deepak.m1 wrote: > > > On 2014/09/04 17:43:01, sky wrote: > > > > Did you sign the Google Individual Contributor License Agreement : > > > > https://developers.google.com/open-source/cla/individual?csw=1 ? > > > @sky PTAL yes sky, I have already signed that as part of Samsung Company, as in the #3 review comment. mailto:"deepak.m1@samsung.com is listed as an authorized contributor in Samsung's list (on the Corporate Signers tab). So, looks fine to me."
deepak.m1@samsung.com changed reviewers: + ben@chromium.org
On 2014/09/08 14:29:48, deepak.m1 wrote: > On 2014/09/06 07:22:40, deepak.m1 wrote: > > On 2014/09/05 06:24:55, deepak.m1 wrote: > > > On 2014/09/05 02:55:59, deepak.m1 wrote: > > > > On 2014/09/04 17:43:01, sky wrote: > > > > > Did you sign the Google Individual Contributor License Agreement : > > > > > https://developers.google.com/open-source/cla/individual?csw=1 ? > > > > > @sky PTAL > yes sky, I have already signed that as part of Samsung Company, as in the #3 > review comment. > mailto:"deepak.m1@samsung.com is listed as an authorized contributor in > Samsung's list (on the Corporate Signers tab). So, looks fine to me." @sky,ben I am already listed as an authorized contributor in Samsung's list (on the Corporate Signers tab) PTAL.
LGTM
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deepak.m1@samsung.com/484133002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 030664d90373fb3652b9e6bf35bdbf1a11229091
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/90a0b2153dfde826d9e351ce2f74c59119ca474a Cr-Commit-Position: refs/heads/master@{#294096} |
