Vertical Tabs: Tab Search Button [chromium/src : main]
Kunal Daftari (Gerrit)
Kunal Daftari added 4 comments![]( "Open in Gerrit")
void SetTabSearchBubbleHost();Location of function in file could be changed.
gfx::Rect button_bounds(For now this sizing seems to be working, but should be evaluated further to make sure it's correct.
bool VerticalTabStripTopButtonContainer::IsPositionInWindowCaption(This code was copied from tab_strip_region_view.cc. It does almost exactly the same thing except uses tab_search_button_ instead of new_tab_button_.
VIEWS_EXPORT extern const ui::ClassProperty<bool>* const kButtonShowsText;Do we want to create a class property or just add a function to do this behavior?
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caroline Rising (Gerrit)
Caroline Rising added 5 comments![]( "Open in Gerrit")
ChromeMenuAction(The ChromeMenuAction helper automatically sets the action's actions::kActionItemPinnableKey to be pinnable. This action should probably instead only be pinnable if features::HasTabSearchToolbarButton() is true.
Feel free to instead wait to address this when you address hiding the tab search button in the toolbar but in that case we should add a todo here to remember to come back to that.
tab_search_button->ShrinkDownThenClearText();Is this still needed?
void VerticalTabStripTopButtonContainer::CreateButtonForTabSearch(
actions::ActionId action_id) {
auto tab_search_button = std::make_unique<views::LabelButton>();
actions::ActionItem* action_item =
actions::ActionManager::Get().FindAction(action_id, root_action_item_);
CHECK(action_item);
action_item->SetProperty(views::kButtonShowsText, false);
action_view_controller_->CreateActionViewRelationship(
tab_search_button.get(), action_item->GetAsWeakPtr());
tab_search_button->ShrinkDownThenClearText();
tab_search_button_ = AddChildView(std::move(tab_search_button));
tab_search_button_->SetHorizontalAlignment(gfx::ALIGN_RIGHT);
}Since we will eventually have two buttons in the container could we generalize this method to be used for both? If we turned it into something like ```views::LabelButton* AddChildButtonFor(actions::ActionId action_id)``` then it could be reused. What do you think?
bool VerticalTabStripTopButtonContainer::IsPositionInWindowCaption(This code was copied from tab_strip_region_view.cc. It does almost exactly the same thing except uses tab_search_button_ instead of new_tab_button_.
Sounds good!
VIEWS_EXPORT extern const ui::ClassProperty<bool>* const kButtonShowsText;Do we want to create a class property or just add a function to do this behavior?
The downside to this approach is if this property changes that won't trigger a call to ActionItemChanngedImpl. Since this would only be used for the top container buttons it might be easier to just create a new simple button that inherits from LabelButton (and its ActionViewInterface) in an anonymous namespace in VerticalTabStripTopButtonContainer and have it not call SetText in its ActionItemChangedImpl.
Related details
- Kunal Daftari
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kunal Daftari (Gerrit)
Kunal Daftari added 5 comments![]( "Open in Gerrit")
The ChromeMenuAction helper automatically sets the action's actions::kActionItemPinnableKey to be pinnable. This action should probably instead only be pinnable if features::HasTabSearchToolbarButton() is true.
Feel free to instead wait to address this when you address hiding the tab search button in the toolbar but in that case we should add a todo here to remember to come back to that.
Done
Location of function in file could be changed.
Done
tab_search_button->ShrinkDownThenClearText();Kunal DaftariIs this still needed?
Not needed! will remove
void VerticalTabStripTopButtonContainer::CreateButtonForTabSearch(
actions::ActionId action_id) {
auto tab_search_button = std::make_unique<views::LabelButton>();
actions::ActionItem* action_item =
actions::ActionManager::Get().FindAction(action_id, root_action_item_);
CHECK(action_item);
action_item->SetProperty(views::kButtonShowsText, false);
action_view_controller_->CreateActionViewRelationship(
tab_search_button.get(), action_item->GetAsWeakPtr());
tab_search_button->ShrinkDownThenClearText();
tab_search_button_ = AddChildView(std::move(tab_search_button));
tab_search_button_->SetHorizontalAlignment(gfx::ALIGN_RIGHT);
}Since we will eventually have two buttons in the container could we generalize this method to be used for both? If we turned it into something like ```views::LabelButton* AddChildButtonFor(actions::ActionId action_id)``` then it could be reused. What do you think?
makes sense!
VIEWS_EXPORT extern const ui::ClassProperty<bool>* const kButtonShowsText;Caroline RisingDo we want to create a class property or just add a function to do this behavior?
The downside to this approach is if this property changes that won't trigger a call to ActionItemChanngedImpl. Since this would only be used for the top container buttons it might be easier to just create a new simple button that inherits from LabelButton (and its ActionViewInterface) in an anonymous namespace in VerticalTabStripTopButtonContainer and have it not call SetText in its ActionItemChangedImpl.
That seems like a relatively complex change. If I remember correctly, I was told that this feature might be used in other areas. Does it make more sense to modify the Label Button class directly to accomodate this behavior?
Related details
- Caroline Rising
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caroline Rising (Gerrit)
Caroline Rising added 3 comments![]( "Open in Gerrit")
// TODO(crbug.com/447146648): Ensure that tab search button is not creatednit let's change this to "is not pinnable". If tab search needs to be able to be seen again (and pinned to the toolbar again) if you switch back to horizontal tabs, then we will need to update the pinnable state for tab search when switching between vertical and horizontal tabs.
// This test checks that we can click the tab search button in the vertical tab
// strip
IN_PROC_BROWSER_TEST_F(VerticalTabStripInteractiveUiTest,
VerifyTabSearchButton) {
browser()
->browser_window_features()
->vertical_tab_strip_state_controller()
->SetVerticalTabsEnabled(true);
RunScheduledLayouts();
RunTestSequence(WaitForShow(kVerticalTabStripTopButtonContainerElementId),
EnsurePresent(kVerticalTabStripTabSearchElementId),
MoveMouseTo(kVerticalTabStripTabSearchElementId),
ClickMouse(), WaitForShow(kTabSearchBubbleElementId));
}nit this test should probably be in a different file since it is not testing anything related to the vertical tabs state controller
return label_button;Could we add the child view here and just return a labelbutton pointer? Also if SetHorizontalAlignment(gfx::ALIGN_RIGHT); will be set for both buttons can that be moved back in here too?
Related details
- Kunal Daftari
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kunal Daftari (Gerrit)
Kunal Daftari added 2 comments![]( "Open in Gerrit")
// TODO(crbug.com/447146648): Ensure that tab search button is not creatednit let's change this to "is not pinnable". If tab search needs to be able to be seen again (and pinned to the toolbar again) if you switch back to horizontal tabs, then we will need to update the pinnable state for tab search when switching between vertical and horizontal tabs.
Done
Could we add the child view here and just return a labelbutton pointer? Also if SetHorizontalAlignment(gfx::ALIGN_RIGHT); will be set for both buttons can that be moved back in here too?
- Caroline Rising
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Caroline Rising (Gerrit)
Caroline Rising added 1 comment![]( "Open in Gerrit")
namespace {
constexpr int kTopButtonContainerHeight = 28;
} // namespaceAs we talked about, defining the button would look something like this
```suggestion
namespace {
constexpr int kTopButtonContainerHeight = 28;
// Class comment here about what this is.
class TopContainerButton : public views::LabelButton {
METADATA_HEADER(TopContainerButton, views::LabelButton)
public:
TopContainerButton() : LabelButton() {}
// add destructor, etc.
// views::LabelButton:
std::unique_ptr<ActionViewInterface> GetActionViewInterface() override {
// create in action view interface
}
};
BEGIN_METADATA(TopContainerButton)
END_METADATA
class TopContainerButtonActionViewInterface : public views::LabelButtonActionViewInterface {
{
public:
// Add constructor/destructor.// LabelButtonActionViewInterface:
void ActionItemChangedImpl(actions::ActionItem* action_item) override {
// Set needed properties.
}
private:
raw_ptr<TopContainerButton> action_view_;
};
} // namespace
```
Related details
- Kunal Daftari
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kunal Daftari (Gerrit)
Kunal Daftari added 2 comments![]( "Open in Gerrit")
namespace {
constexpr int kTopButtonContainerHeight = 28;
} // namespaceAs we talked about, defining the button would look something like this
```suggestion
namespace {
constexpr int kTopButtonContainerHeight = 28;// Class comment here about what this is.
class TopContainerButton : public views::LabelButton {
METADATA_HEADER(TopContainerButton, views::LabelButton)
public:
TopContainerButton() : LabelButton() {}
// add destructor, etc.// views::LabelButton:
std::unique_ptr<ActionViewInterface> GetActionViewInterface() override {
// create in action view interface
}
};
BEGIN_METADATA(TopContainerButton)
END_METADATAclass TopContainerButtonActionViewInterface : public views::LabelButtonActionViewInterface {
{
public:
// Add constructor/destructor.// LabelButtonActionViewInterface:
void ActionItemChangedImpl(actions::ActionItem* action_item) override {
// Set needed properties.
}private:
raw_ptr<TopContainerButton> action_view_;
};
} // namespace
```
Done
VIEWS_EXPORT extern const ui::ClassProperty<bool>* const kButtonShowsText;Caroline RisingDo we want to create a class property or just add a function to do this behavior?
Kunal DaftariThe downside to this approach is if this property changes that won't trigger a call to ActionItemChanngedImpl. Since this would only be used for the top container buttons it might be easier to just create a new simple button that inherits from LabelButton (and its ActionViewInterface) in an anonymous namespace in VerticalTabStripTopButtonContainer and have it not call SetText in its ActionItemChangedImpl.
That seems like a relatively complex change. If I remember correctly, I was told that this feature might be used in other areas. Does it make more sense to modify the Label Button class directly to accomodate this behavior?
- Caroline Rising
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin added 1 comment![]( "Open in Gerrit")
tab_search_bubble_host_ = std::make_unique<TabSearchBubbleHost>(The biggest issue here is this would result in a UAF for the tab_search_toolbar_button_controller since it has a raw ptr to the TabSearchBubbleHost. Moreover, there is also a correctness issue where invoking the toolbar button would open the bubble host in the vertical tab strip entrypoint. So we need to handle these two. Still don't have a great recommendation.
Related details
- Caroline Rising
- Kunal Daftari
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |