Bugzilla

(In reply to Marco Bonardo [:mak] from comment #5)
> Long term, the search bar and urlbar should converge in behavior, both openInTab prefs are also hooked into web-ext, should the new prefs be hooked up too? I'm not 100% sure, and I'd probably suggest to first merge them. That would also require checking how many web-ext are using these browserSettings values effectively.

Thanks for the advice. I can't say I know how to investigate addon usage, but if you can point me in the right direction I'm happy to try. Merging the urlbar & searchbar prefs seems like a worthy goal and the background prefs don't seem as urgent as the popup behavior or making the existing "where to open" behavior consistent across views. So I will make separate bugs for those and assume that background prefs can't be resolved quickly.

> Also the pref name, we have existing .loadInBackground prefs, that suggests this new pref should also be named after those, that allows users to find all releted prefs more easily.

Yeah that does make sense — my original intention was `browser.urlbar.loadInBackground` but when I started adding the same for search, I noticed there's a pref `browser.search.context.loadInBackground` and thought this might confuse users. But I guess that pref only exists because there wasn't already a `browser.search.loadInBackground` to use for that purpose. So maybe they could be merged as well. Another question regarding prefs is, assuming we merge urlbar and search prefs, what the branch name should be.

> Then, of course, there's the modifier key that allows to switch the behavior on the fly, for example while ctrl+click on Windows allows to open in a new tab, ctrl+shift+click opens the tab in background. This may suggest a first good step may be to keep open the panel when we already open in background with the modifier. The modifier of coure complicates the matrix of cases to consider and inverts the behavior of the loadInBackground pref. This is even more complex for search shortcut buttons and switch-to-tab entries, where shift has a different meaning, that should also be analyzed.

Yes it is pretty complex, but I already accounted for one-off search buttons, tab to search results, and each modifier including the existing `tabshifted` behavior. I spent a while working on this so I tried several options, but I think the one reflected in the current patch has the least impact on the current default experience. I believe I tested every urlbar provider, including switch-to-tab.

With switch-to-tab results, behavior is not affected by the existing `openintab` pref, and I wouldn't expect the background pref to affect it either, as it only swaps tab and tabshifted behavior. That is, it's not possible to ctrl+click on a switch-to-tab result due to the action override feature. The only scenario where it would impact behavior is when openintab and openinbackground are both enabled. But that's kind of outside the scope of these changes since it would involve changing the existing behavior for openintab.

In this case, unmodified Enter/click just switches to the tab, which is true irrespective of prefs. And any modifier just changes it to load the URL in the current tab. But that's the same as when any or none of the prefs are enabled. So it doesn't break anything, the question I have is whether it _should_ behave differently. But I don't think that has anything to do with the prefs, because even now, there is no way to open a switch-to-tab result in a new tab or new window. So it already ignores existing modifiers, and adding the pref wouldn't affect that.

If we follow through on bug 1753878, I think a clean solution to the lack of options for switch-to-tab results would be to restrict the action override behavior to the Alt key, just like the canonization behavior. That is, consolidate them into one contextually defined behavior. A switch-to-tab result can't be canonizable, so they aren't stepping on each other. Moreover, I think to make the canonization behavior more obvious (less surprising), we should add some kind of visual indication that the search string is going to be canonized. So by that I mean, treat canonization and "action override" as one thing.

If user presses the Alt key that enables a contextual "alternative" behavior. In the case of switch-to-tab results, that means opening the URL in question in the current tab (and possibly add further Shift and Accel modifiers to allow opening it in a new window or tab instead). And visually, that will be indicated by the "Switch to Tab" action label being replaced with the URL. In the case of regular search results, that means turning the search string into a URL. And visually, that could be indicated by either the search string or the "Search with x" action label being replaced by the canonized URL, with the search string characters highlighted in the URL.

I think that's a separate issue from the load in background thing, but I can make another bug for that (depending on bug 1753878) if you think it's a good idea.

> Summing up, I think the main problem is splitting the work into smaller incremental patches that can be landed and verified separately.
> Those may include keeping the panel open when loading in background using the modifier (no new prefs required, mostly ensuring things around opening in background work), cleaning up the whereToOpen code so later changes are easier to make, figuring out how many webExt are using browserSettings for the current prefs and evaluate merging those prefs into one, introducing the new loadInBackground pref with tests covering behavior with and without the modifier and different kind of results (normal, switch to tab, search shortcut), that will uncover coherence bugs that must have a plan of action, even if it's long term (bug 1536756, bug 1753878).

I think if we are gonna test the outcome of specific result types for a set of modifier/pref combinations, we have to do the same with `openintab` since it has a more dramatic effect for the typical use case I think (unmodified left click/Enter). Does that sound right? I tested for every possible pref and keyboard modifier combination in the test in this patch, but I did it in the same manner as the existing `openintab` test. So it's not very sophisticated, it only checks `where` a result should be opened, not where it actually opens. And neither test validates the one-off search button behavior.

(In reply to Marco Bonardo [:mak] from comment #5)
> Long term, the search bar and urlbar should converge in behavior, both openInTab prefs are also hooked into web-ext, should the new prefs be hooked up too? I'm not 100% sure, and I'd probably suggest to first merge them. That would also require checking how many web-ext are using these browserSettings values effectively.

Thanks for the advice. I can't say I know how to investigate addon usage, but if you can point me in the right direction I'm happy to try. Merging the urlbar & searchbar prefs seems like a worthy goal and the background prefs don't seem as urgent as the popup behavior or making the existing "where to open" behavior consistent across views. So I will make separate bugs for those and assume that background prefs can't be resolved quickly.

> Also the pref name, we have existing .loadInBackground prefs, that suggests this new pref should also be named after those, that allows users to find all releted prefs more easily.

Yeah that does make sense — my original intention was `browser.urlbar.loadInBackground` but when I started adding the same for search, I noticed there's a pref `browser.search.context.loadInBackground` and thought this might confuse users. But I guess that pref only exists because there wasn't already a `browser.search.loadInBackground` to use for that purpose. So maybe they could be merged as well. Another question regarding prefs is, assuming we merge urlbar and search prefs, what the branch name should be.

> Then, of course, there's the modifier key that allows to switch the behavior on the fly, for example while ctrl+click on Windows allows to open in a new tab, ctrl+shift+click opens the tab in background. This may suggest a first good step may be to keep open the panel when we already open in background with the modifier. The modifier of coure complicates the matrix of cases to consider and inverts the behavior of the loadInBackground pref. This is even more complex for search shortcut buttons and switch-to-tab entries, where shift has a different meaning, that should also be analyzed.

Yes it is pretty complex, but I already accounted for one-off search buttons, tab to search results, and each modifier including the existing `tabshifted` behavior. I spent a while working on this so I tried several options, but I think the one reflected in the current patch has the least impact on the current default experience. I believe I tested every urlbar provider, including switch-to-tab.

With switch-to-tab results, behavior is not affected by the existing `openintab` pref, and I wouldn't expect the background pref to affect it either, as it only swaps tab and tabshifted behavior. That is, it's not possible to ctrl+click on a switch-to-tab result due to the action override feature. The only scenario where it would impact behavior is when openintab and openinbackground are both enabled. But that's kind of outside the scope of these changes since it would involve changing the existing behavior for openintab.

In this case, unmodified Enter/click just switches to the tab, which is true irrespective of prefs. And any modifier just changes it to load the URL in the current tab. But that's the same as when any or none of the prefs are enabled. So it doesn't break anything, the question I have is whether it _should_ behave differently. But I don't think that has anything to do with the prefs, because even now, there is no way to open a switch-to-tab result in a new tab or new window. So it already ignores existing modifiers, and adding the pref wouldn't affect that.

If we follow through on bug 1753878, I think a clean solution to the lack of options for switch-to-tab results would be to restrict the action override behavior to the Alt key, just like the canonization behavior. That is, consolidate them into one contextually defined behavior (see bug 1754439). A switch-to-tab result can't be canonizable, so they aren't stepping on each other. Moreover, I think to make the canonization behavior more obvious (less surprising), we should add some kind of visual indication that the search string is going to be canonized. So by that I mean, treat canonization and "action override" as one thing.

If user presses the Alt key that enables a contextual "alternative" behavior. In the case of switch-to-tab results, that means opening the URL in question in the current tab (and possibly add further Shift and Accel modifiers to allow opening it in a new window or tab instead). And visually, that will be indicated by the "Switch to Tab" action label being replaced with the URL. In the case of regular search results, that means turning the search string into a URL. And visually, that could be indicated by either the search string or the "Search with x" action label being replaced by the canonized URL, with the search string characters highlighted in the URL.

I think that's a separate issue from the load in background thing, but I can make another bug for that (depending on bug 1753878) if you think it's a good idea.

> Summing up, I think the main problem is splitting the work into smaller incremental patches that can be landed and verified separately.
> Those may include keeping the panel open when loading in background using the modifier (no new prefs required, mostly ensuring things around opening in background work), cleaning up the whereToOpen code so later changes are easier to make, figuring out how many webExt are using browserSettings for the current prefs and evaluate merging those prefs into one, introducing the new loadInBackground pref with tests covering behavior with and without the modifier and different kind of results (normal, switch to tab, search shortcut), that will uncover coherence bugs that must have a plan of action, even if it's long term (bug 1536756, bug 1753878).

I think if we are gonna test the outcome of specific result types for a set of modifier/pref combinations, we have to do the same with `openintab` since it has a more dramatic effect for the typical use case I think (unmodified left click/Enter). Does that sound right? I tested for every possible pref and keyboard modifier combination in the test in this patch, but I did it in the same manner as the existing `openintab` test. So it's not very sophisticated, it only checks `where` a result should be opened, not where it actually opens. And neither test validates the one-off search button behavior.