Add WebUI backwards and forwards buttons. [chromium/src : main]
Paul Jensen (Gerrit)
Paul Jensen added 1 comment![]( "Open in Gerrit")
aria-haspopup="true"Paul JensenI don't find where Views back/forward button sets this, and don't find this field for back/forward button in chrome://accessibility/. Did I miss something?
In contrast, Views reload button and split tabs button both have GetViewAccessibility().SetHasPopup to set this field.
Qingxin WuGiven the back and forward buttons always allow right-clicking to trigger a popup menu, I think we should be setting this to true, even if the views impls didn't, as it's more correct.
Qingxin Wubut back/forward buttons have a different left click behavior, and adding hasPopup may confuse screen readers?
https://crrev.com/c/6032161 specifically called out that there should not be a haspopup for reload button, since they don't show a menu as a primary
action.
Paul Jensensorry, back/forward butotn, not reload butotn
Paul JensenI'm reaching out to an accessibility expert as it's unclear if aria-haspopup is for primary (i.e. left click) or secondary (i.e. right click) popup menu availability.
Qingxin WuSo it turns out it's not well defined if aria-haspopup refers to the availability of a menu for primary (i.e. left click) or secondary (i.e. right click) actions. Probably worth mimicing what the views impl did, and not setting aria-haspopup as it seems like aria-haspopup may more generally refer to primary actions (and this is a case where the menu is only shown due to secondary actions).
Paul Jensenreload button does set it when it can show context menu with right clicking (https://crsrc.org/c/chrome/browser/ui/views/toolbar/reload_button.cc;drc=3d6ec06c7b2ae9d59c32e3acd2bd39cd0960dd6a;l=320), while back/forward button do not. But in https://crrev.com/c/6032161, the author explicitly mentioned that both reload and back/forward buttons should not set it, but the CL only modified the toolbar, possibly they missed that reload button had a separate place to set it?
Anyways, Views implementation is in conflict itself, and conflict with what they described as well. Worth checking with that CL's owner (oh, they're from microsoft)? Possibly reviewers of it? It can be a bug for Views.
For our implementation, we can probably just align with Views implementation for now.
Ya for this CL I think we should just remove this line
Related details
- Youseff Bourouphel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Jensen (Gerrit)
Paul Jensen added 3 comments![]( "Open in Gerrit")
EXPECT_EQ(l10n_util::GetStringUTF8(IDS_ACCNAME_BACK),can we check the ax::mojom::StringAttribute::kDescription too?
EXPECT_EQ(l10n_util::GetStringUTF8(IDS_ACCNAME_FORWARD),ditto
#endif```suggestion
#endif // BUILDFLAG(IS_MAC)
```
Youseff Bourouphel (Gerrit)
Youseff Bourouphel voted and added 8 comments![]( "Open in Gerrit")
Votes added by Youseff Bourouphel
| Commit-Queue | +1 |
8 comments
Refactor back/forward button to a WebUI component.Youseff BourouphelI'm not sure I would call this a refactor as it's essentially adding (from scratch) the WebUI back/forward buttons. How about:
```suggestion
Add WebUI backwards and forwards buttons.
```
Done
This migrates the back and forward navigation buttons to WebUI,Youseff BourouphelMigration kinda implies this CL is getting rid of the regular back and fowards buttons. How about:
```suggestion
This change adds WebUI back and forward navigation buttons,
```
Done
aria-haspopup="true"Paul JensenI don't find where Views back/forward button sets this, and don't find this field for back/forward button in chrome://accessibility/. Did I miss something?
In contrast, Views reload button and split tabs button both have GetViewAccessibility().SetHasPopup to set this field.
Qingxin WuGiven the back and forward buttons always allow right-clicking to trigger a popup menu, I think we should be setting this to true, even if the views impls didn't, as it's more correct.
Qingxin Wubut back/forward buttons have a different left click behavior, and adding hasPopup may confuse screen readers?
https://crrev.com/c/6032161 specifically called out that there should not be a haspopup for reload button, since they don't show a menu as a primary
action.
Paul Jensensorry, back/forward butotn, not reload butotn
Paul JensenI'm reaching out to an accessibility expert as it's unclear if aria-haspopup is for primary (i.e. left click) or secondary (i.e. right click) popup menu availability.
Qingxin WuSo it turns out it's not well defined if aria-haspopup refers to the availability of a menu for primary (i.e. left click) or secondary (i.e. right click) actions. Probably worth mimicing what the views impl did, and not setting aria-haspopup as it seems like aria-haspopup may more generally refer to primary actions (and this is a case where the menu is only shown due to secondary actions).
Paul Jensenreload button does set it when it can show context menu with right clicking (https://crsrc.org/c/chrome/browser/ui/views/toolbar/reload_button.cc;drc=3d6ec06c7b2ae9d59c32e3acd2bd39cd0960dd6a;l=320), while back/forward button do not. But in https://crrev.com/c/6032161, the author explicitly mentioned that both reload and back/forward buttons should not set it, but the CL only modified the toolbar, possibly they missed that reload button had a separate place to set it?
Anyways, Views implementation is in conflict itself, and conflict with what they described as well. Worth checking with that CL's owner (oh, they're from microsoft)? Possibly reviewers of it? It can be a bug for Views.
For our implementation, we can probably just align with Views implementation for now.
Ya for this CL I think we should just remove this line
Done
protected onClick_(e: MouseEvent) {Youseff BourouphelThis elements handling of long-press is quite different from the reload button's: e.g. reload button doesn't handle onClick and reload button's onPointerUp seems to handle clicks and also has the code from lines 77-79 in this file. Is there a reason that this element is different? if not, can we make it more similar to reload button? we can move the duplicated code into a separate class/mixin in a CL later, but having the same code will avoid having different bugs in each control.
Done
if (e.button !== BUTTON_LEFT) { // Only left click for long press
return;
}Youseff Bourouphelhmm, I think a long middle click also brings up the context menu (like reload button)?
Done
can we check the ax::mojom::StringAttribute::kDescription too?
Done
EXPECT_EQ(l10n_util::GetStringUTF8(IDS_ACCNAME_FORWARD),Youseff Bourouphelditto
Done
#endifYouseff Bourouphel```suggestion
#endif // BUILDFLAG(IS_MAC)
```
Done
Related details
- Paul Jensen
- Qingxin Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Jensen (Gerrit)
Paul Jensen added 12 comments![]( "Open in Gerrit")
I think if the merge conflicts are fixed and my comments are addressed, I'm ready to +1 this CL.
} else if (back_) {To match existing code
```suggestion
} else {
```
} else if (forward_) {To match existing code
```suggestion
} else {
```
return browser_controls_api::mojom::ButtonState::New(enabled_, visible_);```suggestion
return browser_controls_api::mojom::ButtonState::New(/*enabled=*/enabled_,
/*visible=*/visible_);
```
EXPECT_EQ(l10n_util::GetStringUTF8(IDS_ACCNAME_BACK),
back.GetStringAttribute(ax::mojom::StringAttribute::kName));
EXPECT_EQ(l10n_util::GetStringUTF8(IDS_TOOLTIP_BACK),
back.GetStringAttribute(ax::mojom::StringAttribute::kDescription));nit: sometimes for testing purposes it's better to put in fixed constants, e.g. "Reload this page" on line 343 above, to ensure we're not perpetuating an error from the strings file. Not critical so marking resolved. Ditto for lines 370-375 below
find_criteria.name = "Reload";Is there a reason to move this code down here? it might be good to move this reload test code next to the other reload test code on line 346. Also, why is this changed to a RunUntil? can we also change it back to the pre-existing code?
ASSERT_TRUE(base::test::RunUntil([&]() {```suggestion
EXPECT_TRUE(base::test::RunUntil([&]() {
```
ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(),
GURL(chrome::kChromeUINewTabURL)));
ASSERT_TRUE(
ui_test_utils::NavigateToURL(browser(), GURL("chrome://version")));why does this navigate twice rather than just once?
// Check visibility via JS.
EXPECT_TRUE(WaitForButtonVisible(web_view->GetWebContents(), kBackSelector));
// Forward button should be visible but disabled initially (no forward
// history).
EXPECT_TRUE(
WaitForButtonVisible(web_view->GetWebContents(), kForwardSelector));should this check the button disabled status like we did on line 496 now that the back button should be enabled?
EXPECT_TRUE(ditto now that the forward button should be enabled?
#if BUILDFLAG(IS_MAC)nit: Rather than using this MAYBE logic, it might be simpler to say
```
#if !BUILDFLAG(IS_MAC)
TEST_F(BrowserControlsServiceTest, Back_CtrlClick) {
...}
#else
TEST_F(BrowserControlsServiceTest, Back_MetaClick) {
...}
```
ditto for line 326
bool back_button_hovered_ = false;This looks unused; we should probably check that it gets set in BrowserControlsServiceTest.BackButtonHovered by adding an accessor like is_split_tab()
Related details
- Qingxin Wu
- Youseff Bourouphel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 10 comments![]( "Open in Gerrit")
To match existing code
```suggestion
} else {
```
Done
To match existing code
```suggestion
} else {
```
Done
return browser_controls_api::mojom::ButtonState::New(enabled_, visible_);```suggestion
return browser_controls_api::mojom::ButtonState::New(/*enabled=*/enabled_,
/*visible=*/visible_);
```
Done
Is there a reason to move this code down here? it might be good to move this reload test code next to the other reload test code on line 346. Also, why is this changed to a RunUntil? can we also change it back to the pre-existing code?
Done
```suggestion
EXPECT_TRUE(base::test::RunUntil([&]() {
```
Acknowledged
ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(),
GURL(chrome::kChromeUINewTabURL)));
ASSERT_TRUE(
ui_test_utils::NavigateToURL(browser(), GURL("chrome://version")));why does this navigate twice rather than just once?
Done
// Check visibility via JS.
EXPECT_TRUE(WaitForButtonVisible(web_view->GetWebContents(), kBackSelector));
// Forward button should be visible but disabled initially (no forward
// history).
EXPECT_TRUE(
WaitForButtonVisible(web_view->GetWebContents(), kForwardSelector));should this check the button disabled status like we did on line 496 now that the back button should be enabled?
Done
ditto now that the forward button should be enabled?
Done
nit: Rather than using this MAYBE logic, it might be simpler to say
```
#if !BUILDFLAG(IS_MAC)
TEST_F(BrowserControlsServiceTest, Back_CtrlClick) {
...}
#else
TEST_F(BrowserControlsServiceTest, Back_MetaClick) {
...}
```
ditto for line 326
Done
This looks unused; we should probably check that it gets set in BrowserControlsServiceTest.BackButtonHovered by adding an accessor like is_split_tab()
- Paul Jensen
- Qingxin Wu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Jensen (Gerrit)
Paul Jensen voted and added 2 comments![]( "Open in Gerrit")
Votes added by Paul Jensen
| Code-Review | +1 |
2 comments
#if BUILDFLAG(IS_MAC)
TEST_F(BrowserControlsServiceTest, Back_MetaClick) {
service().Back(
{browser_controls_api::mojom::ClickDispositionFlag::kMetaKeyDown});
EXPECT_EQ(IDC_BACK, toy_browser().received_commands().back().command_id);
EXPECT_EQ(WindowOpenDisposition::NEW_BACKGROUND_TAB,
toy_browser().received_commands().back().disposition);
}
#else
TEST_F(BrowserControlsServiceTest, Back_CtrlClick) {
service().Back(
{browser_controls_api::mojom::ClickDispositionFlag::kControlKeyDown});
EXPECT_EQ(IDC_BACK, toy_browser().received_commands().back().command_id);
EXPECT_EQ(WindowOpenDisposition::NEW_BACKGROUND_TAB,
toy_browser().received_commands().back().disposition);
}
#endif```suggestion
TEST_F(BrowserControlsServiceTest, Back_MetaOrCtrlClick) {
service().Back(
#if BUILDFLAG(IS_MAC)
{browser_controls_api::mojom::ClickDispositionFlag::kMetaKeyDown});
#else
{browser_controls_api::mojom::ClickDispositionFlag::kControlKeyDown});
#endif // BUILDFLAG(IS_MAC)
EXPECT_EQ(IDC_BACK, toy_browser().received_commands().back().command_id);
EXPECT_EQ(WindowOpenDisposition::NEW_BACKGROUND_TAB,
toy_browser().received_commands().back().disposition);
}
```
#if BUILDFLAG(IS_MAC)
TEST_F(BrowserControlsServiceTest, Forward_MetaClick) {
service().Forward(
{browser_controls_api::mojom::ClickDispositionFlag::kMetaKeyDown});
EXPECT_EQ(IDC_FORWARD, toy_browser().received_commands().back().command_id);
EXPECT_EQ(WindowOpenDisposition::NEW_BACKGROUND_TAB,
toy_browser().received_commands().back().disposition);
}
#else
TEST_F(BrowserControlsServiceTest, Forward_CtrlClick) {
service().Forward(
{browser_controls_api::mojom::ClickDispositionFlag::kControlKeyDown});
EXPECT_EQ(IDC_FORWARD, toy_browser().received_commands().back().command_id);
EXPECT_EQ(WindowOpenDisposition::NEW_BACKGROUND_TAB,
toy_browser().received_commands().back().disposition);
}
#endif```suggestion
TEST_F(BrowserControlsServiceTest, Forward_MetaOrCtrlClick) {
service().Forward(
#if BUILDFLAG(IS_MAC)
{browser_controls_api::mojom::ClickDispositionFlag::kMetaKeyDown});
#else
{browser_controls_api::mojom::ClickDispositionFlag::kControlKeyDown});
#endif // BUILDFLAG(IS_MAC)
EXPECT_EQ(IDC_FORWARD, toy_browser().received_commands().back().command_id);
EXPECT_EQ(WindowOpenDisposition::NEW_BACKGROUND_TAB,
toy_browser().received_commands().back().disposition);
}
```
Related details
- Qingxin Wu
- Youseff Bourouphel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Youseff Bourouphel
| Commit-Queue | +1 |
2 comments
#if BUILDFLAG(IS_MAC)
TEST_F(BrowserControlsServiceTest, Back_MetaClick) {
service().Back(
{browser_controls_api::mojom::ClickDispositionFlag::kMetaKeyDown});
EXPECT_EQ(IDC_BACK, toy_browser().received_commands().back().command_id);
EXPECT_EQ(WindowOpenDisposition::NEW_BACKGROUND_TAB,
toy_browser().received_commands().back().disposition);
}
#else
TEST_F(BrowserControlsServiceTest, Back_CtrlClick) {
service().Back(
{browser_controls_api::mojom::ClickDispositionFlag::kControlKeyDown});
EXPECT_EQ(IDC_BACK, toy_browser().received_commands().back().command_id);
EXPECT_EQ(WindowOpenDisposition::NEW_BACKGROUND_TAB,
toy_browser().received_commands().back().disposition);
}
#endif```suggestion
TEST_F(BrowserControlsServiceTest, Back_MetaOrCtrlClick) {
service().Back(
#if BUILDFLAG(IS_MAC)
{browser_controls_api::mojom::ClickDispositionFlag::kMetaKeyDown});
#else
{browser_controls_api::mojom::ClickDispositionFlag::kControlKeyDown});
#endif // BUILDFLAG(IS_MAC)
EXPECT_EQ(IDC_BACK, toy_browser().received_commands().back().command_id);
EXPECT_EQ(WindowOpenDisposition::NEW_BACKGROUND_TAB,
toy_browser().received_commands().back().disposition);
}
```
Fix applied.
#if BUILDFLAG(IS_MAC)
TEST_F(BrowserControlsServiceTest, Forward_MetaClick) {
service().Forward(
{browser_controls_api::mojom::ClickDispositionFlag::kMetaKeyDown});
EXPECT_EQ(IDC_FORWARD, toy_browser().received_commands().back().command_id);
EXPECT_EQ(WindowOpenDisposition::NEW_BACKGROUND_TAB,
toy_browser().received_commands().back().disposition);
}
#else
TEST_F(BrowserControlsServiceTest, Forward_CtrlClick) {
service().Forward(
{browser_controls_api::mojom::ClickDispositionFlag::kControlKeyDown});
EXPECT_EQ(IDC_FORWARD, toy_browser().received_commands().back().command_id);
EXPECT_EQ(WindowOpenDisposition::NEW_BACKGROUND_TAB,
toy_browser().received_commands().back().disposition);
}
#endif```suggestion
TEST_F(BrowserControlsServiceTest, Forward_MetaOrCtrlClick) {
service().Forward(
#if BUILDFLAG(IS_MAC)
{browser_controls_api::mojom::ClickDispositionFlag::kMetaKeyDown});
#else
{browser_controls_api::mojom::ClickDispositionFlag::kControlKeyDown});
#endif // BUILDFLAG(IS_MAC)
EXPECT_EQ(IDC_FORWARD, toy_browser().received_commands().back().command_id);
EXPECT_EQ(WindowOpenDisposition::NEW_BACKGROUND_TAB,
toy_browser().received_commands().back().disposition);
}
```
- Paul Jensen
- Youseff Bourouphel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 1 comment![]( "Open in Gerrit")
Adding Eshwar for views review, as they have seen this before.
Related details
- Eshwar Stalin
- Paul Jensen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Eshwar Stalin
| Code-Review | +1 |
1 comment
void SetForwardButtonVisible(bool visible);nit: Visibility
Related details
- Paul Jensen
- Youseff Bourouphel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 1 comment![]( "Open in Gerrit")
void SetForwardButtonVisible(bool visible);Youseff Bourouphelnit: Visibility
Done
Related details
- Eshwar Stalin
- Paul Jensen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youseff Bourouphel (Gerrit)
Youseff Bourouphel added 1 comment![]( "Open in Gerrit")
Adding IPC for all the mojo changes and Nico for the resource_ids.spec
Related details
- Chromium IPC Reviews
- Eshwar Stalin
- Nico Weber
- Paul Jensen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ort...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): ort...@chromium.org
Reviewer source(s):
ort...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Eshwar Stalin
- Giovanni Ortuno Urquidi
- Nico Weber
- Paul Jensen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Giovanni Ortuno Urquidi
| Code-Review | +1 |
1 comment
- Eshwar Stalin
- Nico Weber
- Paul Jensen
- Youseff Bourouphel