Issue 1268 - Correctly mimic Chrome's behavior when opening the options page

Issue 1268 - Correctly mimic Chrome's behavior when opening the options page

[Wladimir Palant, 2014-08-28 16:14:08]

So we are essentially giving up on the compatibility layer?

Fact is, this won't really mimic Chrome's behavior. According to documentation,
chrome.tabs.create() is supposed to open tabs in the current window (= the
window running code right now) by default. Since our code is running from a
pop-up, the current window happens to be the same as the last focused window -
lucky us. However, if the current window is an incognito window then for
extension pages (and only those!) chrome.tabs.create() will automatically
replace it by the last focused window that isn't incognito.

From all I can tell, there is no way to get the last focused window that isn't
incognito via extension APIs. So the only way to mimic Chrome's behavior is to
actually open the page and then check which window it opened in - we can then
close it immediately if we find another tab there.

IMHO the simpler solution would be to check all tabs in all windows instead
(yes, this was discussed already). If we find the options open *somewhere* we
merely need to focus the corresponding tab and window. This can be implemented
within the compatibility layer, we merely need to add a way to focus a page.

[Sebastian Noack, 2014-08-28 16:43:15]

On 2014/08/28 16:14:08, Wladimir Palant wrote:
> So we are essentially giving up on the compatibility layer?

Addressing this issue, would require to add a fair amount of new APIs to the
abstraction layer, which aren't needed anywhere else. So I considered it a
better approach to just add a high-level showOptions() function to the
abstraction layer.

> Fact is, this won't really mimic Chrome's behavior. According to
documentation,
> chrome.tabs.create() is supposed to open tabs in the current window (= the
> window running code right now) by default. Since our code is running from a
> pop-up, the current window happens to be the same as the last focused window -
> lucky us. However, if the current window is an incognito window then for
> extension pages (and only those!) chrome.tabs.create() will automatically
> replace it by the last focused window that isn't incognito.
> 
> From all I can tell, there is no way to get the last focused window that isn't
> incognito via extension APIs. So the only way to mimic Chrome's behavior is to
> actually open the page and then check which window it opened in - we can then
> close it immediately if we find another tab there.
>
> IMHO the simpler solution would be to check all tabs in all windows instead
> (yes, this was discussed already). If we find the options open *somewhere* we
> merely need to focus the corresponding tab and window. This can be implemented
> within the compatibility layer, we merely need to add a way to focus a page.

I'm aware that it doesn't mimics Chrome's behavior to 100%, but it is closer.
And regardless what Chrome does, the current behavior when using an incognito
window is just broken in Adblock Plus.

However, keeping track of the last focused non-incognito window, requires a fair
amount of complexity, and would be only a tiny UX improvement, addressing a very
rare corner case.

But showing the options page in the same window if possible, is a very useful
UI/UX feature, and doesn't require much too much extra complexity, IMO.

[Wladimir Palant, 2014-08-28 17:35:44]

On 2014/08/28 16:43:15, Sebastian Noack wrote:
> But showing the options page in the same window if possible, is a very useful
> UI/UX feature, and doesn't require much too much extra complexity, IMO.

This isn't about where we show the options page - this part won't change. It is
merely about whether we find an options page already open in a different window
or not. Your current solution introduces inconsistent behavior: we will normally
look for the options page in the current window only, unless the current window
is an incognito window in which case we look for it everywhere. I suggest always
looking for the options page everywhere, which IMHO is more logical for the
user.

[Sebastian Noack, 2014-08-29 16:35:07]

Let me remind you, that back then when I introduced that code Felix, you and
myself agreed that we should stick to the way Chrome does things. Since this the
behavior the user is used to and expects.

And even though the behavior this patch implements, still isn't 100% what Chrome
does, it is very close to it, and we seem to agree that adding even more
complexity wouldn't be worth.

Also I agree with Chrome, that showing the options page in a different window is
a bad user experience. There are some situations where you can't bring an other
browser window into the foreground (e.g. when that window is on another monitor
or workspace, or if you are using a window manager that doesn't support that at
all). And even when that isn't an issue, I don't want windows that I put into
background for a reason, to unexpectedly pop up again. Only in the case of
incognito windows that is the best thing you can do, since incognito windows
can't access extension pages.

[Wladimir Palant, 2014-08-29 19:32:14]

Yes, I remember this discussion - and I consider the issue at hand a reminder
that the decision back then might no longer be valid. IMHO you are trying way
too hard and making this more complicated than necessary, for a very rare
scenario where this might indeed hurt usability.

Seeing that we don't get anywhere arguing, I'm asking Felix to look at this.

[Sebastian Noack, 2014-08-29 19:36:01]

(looping in Felix)

[Felix Dahlke, 2014-09-04 16:44:57]

On 2014/08/29 19:36:01, Sebastian Noack wrote:
> (looping in Felix)

Sorry for the delay. I'll try to sum things up first, CMIIW:

It seems the main question is: What should happen if the user opens the ABP
options page from a non-incognito window, when the options page is already open
in another window.

You discussed two options:

1. Mimic the way Chrome opens the ABP options page from the extensions page and
open a fresh options page in the current window.
2. Always bring the ABP options page to the foreground if it's open in another
window, incognito or not.

I think we should go with #1. My reasons:

1. It's consistent with how things work on Chrome - being consistent with the
platform is more important than being consistent across platforms IMO.
2. I actually agree with Chrome and Sebastian that it's the best UX. Bringing
random windows to the front would throw people off from what they're doing right
now, and it could even be a privacy problem.
3. This patch doesn't add too much complexity IMO. It's too soon to generalise
with only two ext implementations, but I'm convinced we can avoid duplication if
this works the exact same way on two platforms.

Note that #2 applies to how we'd open the options page from an incognito window,
too, but less so IMO: Whether we open the last focused non-incognito window or
any other window - we'll throw the user out of context anyway. And I suppose we
can also presume people shouldn't be worried about their privacy too much while
using incognito mode (plus opening the last focused window would still be a
potential privacy problem).

[Wladimir Palant, 2014-09-04 17:12:43]

Actually, I think that there is a better solution - we can stop relying on the
undocumented chrome.tabs.create() behavior that will "redirect" the options page
to a different window. We can instead check the current window, and if it is an
incognito window we can choose some other (non-incognito) window to open the
Options page in it. Then we can check whether that window already has our
Options page in it, exactly the same way we did it before.

[Felix Dahlke, 2014-09-04 22:24:56]

On 2014/09/04 17:12:43, Wladimir Palant wrote:
> Actually, I think that there is a better solution - we can stop relying on the
> undocumented chrome.tabs.create() behavior that will "redirect" the options
page
> to a different window. We can instead check the current window, and if it is
an
> incognito window we can choose some other (non-incognito) window to open the
> Options page in it. Then we can check whether that window already has our
> Options page in it, exactly the same way we did it before.

You mean we should do this in the Chrome implementation of ext.pages.open
instead of moving showOptions into ext and implementing the logic only there?
Sounds good to me, I guess we'd have the same problem with share links and links
in notifications anyway.

[Sebastian Noack, 2014-09-05 11:07:13]

On 2014/09/04 17:12:43, Wladimir Palant wrote:
> Actually, I think that there is a better solution - we can stop relying on the
> undocumented chrome.tabs.create() behavior that will "redirect" the options
page
> to a different window. We can instead check the current window, and if it is
an
> incognito window we can choose some other (non-incognito) window to open the
> Options page in it. Then we can check whether that window already has our
> Options page in it, exactly the same way we did it before.

I thought you were concerned about the complexity my patch is adding. However,
what you are suggesting would add way more complexity.

Also I think relying on the behavior of chrome.tabs.create(), is exactly what we
should do. Whether its documented or not, it can be considered the default
behavior. And the default behavior is what we attempt to mimic.


On 2014/09/04 22:24:56, Felix H. Dahlke wrote:
> You mean we should do this in the Chrome implementation of ext.pages.open
> instead of moving showOptions into ext and implementing the logic only there?

I don't think that anything Wladimir said implies that. Neither do I think this
is a good idea. ext.pages.open() is usually used to open links, like they would
when you click on them on a regular web page. So what the user expects in that
case is that a new tab is created regardless whether that page is already open.

[Wladimir Palant, 2014-10-27 12:18:54]

LGTM