-
Notifications
You must be signed in to change notification settings - Fork 71
Proposal: Focus management API #817
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for the proposal! Besides the method |
|
When writing the first draft of this proposal I didn't have a clear picture for how we should handle errors in |
My intent was that the promise returned by the @hanguokai, can you describe a use case for wanting to query the browser's current focus? I didn't see a need for that in the use case that motivated the proposal. |
This comment was marked as outdated.
This comment was marked as outdated.
|
@dotproto Preliminary review of the proposal, I'm sorry I object to the API design. Such a call is very strange and cumbersome: browser.tabs.focus({area: "sidebar"})The main reasons are:
I think possible alternative designs are:
I tend to the latter because it's intuitive enough and simple enough that you don't actually need to pass in any parameters. The second problem has to be mentioned here, that is, we should not pass in options such as The third point is that above I also mentioned |
| ## Security and Privacy | ||
|
|
||
| Focus management presents a moderate risk to end users. While changing the user agent’s current focus does not grant access to any data in itself, it greatly increases the risk that a user will perform an unintended action. When combined with [user activation](https://html.spec.whatwg.org/multipage/interaction.html#tracking-user-activation), this capability can be used to receive escalated permissions or to perform a restricted operation without the user’s express permission. | ||
|
|
||
| As such, this proposal intentionally does not entertain the possibility of allowing extensions to focus user agent UI surfaces such as an extension’s action button. When considering future expansion of focus management capabilities, the authors recommend gating the ability to focus dangerous such UI surfaces behind a permission with a warning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The security concern here is a misinterpretation of user activation.
Because we actually already have the relevant APIs for opening the extensions popup or sidebar:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And these APIs have the user activation restriction applied already:
browser.action.openPopup();
Uncaught (in promise) Error: action.openPopup may only be called from a user input handler
browser.sidebarAction.open();
Uncaught (in promise) Error: sidebarAction.open may only be called from a user input handlerThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There have been a few discussions in this group about removing the user activation requirement from action.openPopup(), sidebarAction.open(), and sidePanel.open(). To the best of my knowledge browsers are aligned on removing the requirement from action.openPopup(), but I'm not sure there's consensus around the sidebar concepts.
- Add support for opening side panel without user interaction #472
- Ensure consistency of
action.openPopupAPI across browsers #160
To be clear, toggling the visibility of the sidebar and popup are not the concern here. User interactions with browser controlled extension UI surfaces apply a user activation (also refereed to as a user gesture) flag in the event handler. Many web and extension APIs and are also gated behind user activation, such as invoking permissions.request() to request an optional (or disabled) permission.
More importantly, though, user interactions with browser controlled extension UI surfaces also grant activeTab. This gives the extension temporary host permission access to the page on which the grant occured, which includes the ability to inject scripts into a page. In situations where an extension doesn't request activeTab but does request broad host permissions, users can use browser settings to configure the extension to only "run on click." In this mode, extensions behave similarly to the activeTab: they will only inject content scripts when an appropriate trigger is invoked by the user and the host permissions granted are revoked after navigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that explains the broader considerations.
Because it initially sounds like preventing the open/focus popup (action button).
It should be mentioned that if intend to focus only the tab-top-window, it is unnecessary to obtain host permissions.
|
Another issue is like what I mentioned in my initial comment in #693 (comment), some of the current APIs automatically shift focus when we call them, and we should probably add options to those APIs to prevent focus shifting. Although we could for example do so (to keep focus still on await browser.tabs.update(tabId, { active: true }); // focus automatically shifts to `tab` (page)
await browser.sidebarAction.focus(); // manually shift the focus back to `sidebar`But it has two focus transfer, which can be unexpected or delayed, ultimately reducing the user experience. So a better alternative might be add a browser.tabs.update(tabId, { active: true, focus: false });Note
And we may want to add corresponding property to And I think this maybe a better alternative to the above |
|
Currently, calling browser.sidebar.open({
focus: true,
});This has a number of benefits:
Considering It could even be argued focussing the sidebar after |
Why does it cause an error when sidebar is closed? Do you want to express that it is called again when it is already opened?
What are you referring to?
Actually I don't think it's awkward: await browser.sidebarAction.open();
await browser.sidebarAction.focus();But I don't object to adding options to the
I can see the logic of the argument, but I can also immediately think of some negative examples, such as if sidebar is just used to show some information, then it does not need to have to get focus. |
The W3C TAG's Writing Promise-Using Specifications states that "rejection reasons must be Error instances" (4.1.2.) and "rejections must be used for exceptional situations" (4.1.3.). That provides some pretty strong guidance on how rejections should be handled. The thing I am unsure about is what failure cases should be considered exceptional. I think it makes sense to reject if the provided
No need to apologize. We're here to share and discuss feedback :) I agree with the critiques you made of exposing this capability on the I had this method in I hadn't given the idea of creating different That said, the longer I sit with this idea the more I like it. It also addresses something that's bothered me about my currently proposed approach: if the options object grows to accommodate the needs of arbitrary UI surfaces, it may become very difficult for developers (and maintainers) to reason about. Namespace specific
I was envisioning the currently proposed const focusTab = async (tabId) => {
const tab = await browser.tabs.update(tabId, { active: true });
browser.windows.update(tab.windowId, { focused: true });
}If we have namespace specific methods that only operate on the current window and tab, this would pattern would become more cumbersome: const focusTabSidebar = async (tabId) => {
const tab = await browser.tabs.update(tabId, { active: true });
browser.windows.update(tab.windowId, { focused: true });
if (browser.sidebarAction) {
await browser.sidebarAction.open();
await browser.sidebarAction.focus();
} else {
await browser.sidePanel.open();
await browser.sidePanel.focus();
}
}This could be simplified in the future if we align on a common
I touched on this idea in the Future Work > Ancillary UI surfaces section of the proposal. We also briefly discussed it in today's WECG meeting. @Rob--W raised serious concerns about allowing extensions to focus the address bar, but we didn't have time to begin exploring those concerns. |
|
@carlosjeurissen, I see how adding a focus option to
That's basically the behavior described in the current proposal (highlight added for clarity):
The |
Thanks for citing the specification, but I think it is exactly in line with my understanding. In this particular case, when you call the Otherwise, it is a nightmare for developers, both "to catch error when throw" and "to check the return value when resolved" to confirm whether
Simply put, I agree.
I think there are several problems here:
Yes I saw this in later reading. I really didn't think of a use case at the moment, but it is still necessary to consider this possibility in design. So maybe it's not worth too much attention before fully considering the use cases and risks.
Thanks, this explains my question, which makes sense for the
OK, thanks and that explains part of it. I think the concept of user intuition is that one interaction (such as a click) will only get a certain part/element visible in the current screen to focus, or switch to another view. In fact, I am disappointed with the processing of the first click of the current non-activated window, different performances in different applications and systems. So I can see the benefits of this for powerful automation and convenience. And if this is the case, you may need to design all the advanced and complex focus management systems separately in But I think this seems a little bit beyond what we are currently trying to solve. |
|
*** waiting with bated breath *** |
An example of a web API which resolves even on some errors is the Fetch API. Which can resolve while the server responds with a 404 error. In extension space, APIs like As for if this API should reject or resolve when attempting to focus, we can look at existing behaviour in other APIs like In general it seems blocking UI patterns like that needs some more consideration and documentation. For example, with these Google sign-in prompts, extension popups can not be opened not even when the user clicks an extension icon. Which might be a bug @oliverdunk. |
|
@carlosjeurissen Thanks for providing more references.
In this case, fetching data about the error page may be expected behavior, so reject is obviously inappropriate. It's not a error of the
I think in this case it's because it will be called multiple times during the extension lifecycle, it's more of a query to return rather than a task to resolve. In particular, it only actually pops up the authorization dialog to the user the first time. And the In any case, thanks you all for putting forward these references and I will think more carefully and critically about expressing my views in the future. |
That's true. On the other hand, if you try try to perform a cross origin request and the response doesn't have the right CORS headers set the Between the discussion here and some offline followup conversations I had with browser engineers, it seems like the best path forward is to revise this proposal to use per-namespace |
Thank you for all your work and it's great to see it continue to be pushed forward. I think "focus management" will have more advanced and meticulous improvements in the future, not only at the browser level, but also at the system/window level. But as far as solving the problem we need right now, per-namespace Note that For this, I still think that using |
|
I'm now more of the opinion that adding a Currently we could check if a stored const tabId = 1;
const focusedTab = await browser.tabs.query({active: true, lastFocusedWindow: true})
if(focusedTab.id === tabId) {
const tab = await browser.tabs.get(tabId);
// tab...
}or const tabId = 1;
const tab = await browser.tabs.get(tabId);
const win = await browser.windows.get(tab.windowId);
if (win.focused && tab.active) {
// tab...
}They're all a bit troublesome, and that doesn't include error handling. Why can't we directly: const tabId = 1;
const tab = await browser.tabs.get(tabId);
if (tab.focused) {
// tab...
}More to the point, it solves the problems I mentioned above without API breaking changes: #817 (comment) |
If we're going for set of API changes and only introducing a At the moment, I think the path we take for the first revision is contingent on how likely we are to either introduce a
Sorry, I didn't follow this comment. How does introducing a |
|
@dotproto Thanks for the reply.
First of all, I'm sorry for the point I wrote in all the comments above. I should use
I admit this creates inconsistencies with other namespaces, but this is due to the complexity of the await browser.windows.update( windowId, { focused: true } );So do we also need to implement I think there is inconsistency of If other namespaces have similar complexity and have appropriate places to place, I think maybe similar tweaks make sense, for example I've seen that
In fact, if we want to use More importantly, if we want to consider more meticulous focus management rather than just switching focus between different blocks (tab, sidebar, etc.), that is, if we want to consider focus management inside the block (like frames or even elements you said), I strongly believe that we should use the brand new namespace Another point is that I'm not sure if a use case like focus sub-frames is currently something to consider or necessary. I think the current focus is mainly to switch In other words, I think what we need to urgently address at this point should just be refining the current APIs to provide a way to switch focus between blocks (tab, sidebar, etc.), whether it's In fact I don't even mind having both After this is done, we can consider using the new namespace
Still the use case I mentioned above, how do we use |
This proposal introduces a
tab.focus()method and supportingtabnamespace properties.Status
This is the first draft of the proposal. It is not entirely contiguous, but initial feedback is welcome.