Private Windows shouldn't disable switch-to-tab by disabling all autocomplete actions

VERIFIED FIXED in Firefox 36

Status

()

Firefox
Location Bar
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Unfocused, Assigned: ttaubert)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 36
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox36 verified, firefox37 verified)

Details

Attachments

(3 attachments, 7 obsolete attachments)

2.55 KB, patch
mak
: review+
Details | Diff | Splinter Review
539 bytes, patch
mak
: review+
Details | Diff | Splinter Review
13.85 KB, patch
mak
: review+
Details | Diff | Splinter Review
Looks like Private Windows abuse the autocomplete API by disabling *all* autocomplete actions, when it only wants to disable switch-to-tab. This is killing all new autocomplete functionality in private windows.
Flags: qe-verify+
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Marco: This is an autocomplete hard blocker, so needs done this iteration.
Blair, do you have a suggested way to handle this? That may help someone willing to jump into fixing this bug.
I don't think we have support to disable single actions currently, right?

Updated

3 years ago
Flags: needinfo?(mmucci)
Blocks: 1075812

Updated

3 years ago
No longer blocks: 1075812
Duplicate of this bug: 1075812

Updated

3 years ago
Blocks: 995091
(Assignee)

Comment 4

3 years ago
FTR, disabling switch-to-tab in private windows was done in bug 816527.
Blocks: 816527
(Assignee)

Comment 5

3 years ago
Created attachment 8501455 [details] [diff] [review]
0001-Bug-1075450-Disable-only-switch-to-tab-instead-of-al.patch

How about this? I feel like adding a mechanism to disable arbitrary actions would be a little over-engineered.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8501455 - Flags: review?(mak77)
(Assignee)

Updated

3 years ago
Iteration: --- → 35.3
(Assignee)

Comment 6

3 years ago
Created attachment 8501921 [details] [diff] [review]
0001-Bug-1075450-Disable-only-switch-to-tab-instead-of-al.patch, v2

Should probably make that work with the old nsAutocomplete as well.
Attachment #8501455 - Attachment is obsolete: true
Attachment #8501455 - Flags: review?(mak77)
Attachment #8501921 - Flags: review?(mak77)
I assume we might want a test here, checking switch to tab is disabled but another action is not... That would be useful regardless of the approach we take.
fwiw, I agree we should not go crazy about whitelisting actions.
Comment on attachment 8501921 [details] [diff] [review]
0001-Bug-1075450-Disable-only-switch-to-tab-instead-of-al.patch, v2

Review of attachment 8501921 [details] [diff] [review]:
-----------------------------------------------------------------

So, first, we need a test. the original patch implementing this should have not been r+ed without a test. Or we would have catched this regression first!

this should also fix this code in UnifiedComplete, by removing the switchToTabQuery:

let queries = [ this._adaptiveQuery,
                this._switchToTabQuery,	
                this._searchQuery ];

and the equivalent in nsPlacesAutoComplete.

note that Bug 530209 is basically doing the same thing (you can look at the patch there for how it's doing), so if that lands first this could reuse that code, or viceversa.

The approach is ok imo. note that this approach (adding more to searchparams) was discarded in bug 816527, but that imo was because at that time we only had a single action, so it was indeed easier to disable actions completely than to special case switch to tab. I think today this approach makes sense cause we have many actions.

::: browser/base/content/browser.js
@@ +6946,5 @@
>      }
>  
>      if (gURLBar &&
>          !PrivateBrowsingUtils.permanentPrivateBrowsing) {
>        // Disable switch to tab autocompletion for private windows 

while here, please fix this trailing space.

@@ +6947,5 @@
>  
>      if (gURLBar &&
>          !PrivateBrowsingUtils.permanentPrivateBrowsing) {
>        // Disable switch to tab autocompletion for private windows 
>        // (not for "Always use private browsing" mode)

...and rephrase to remove the pointless parenthesis :)

@@ +6949,5 @@
>          !PrivateBrowsingUtils.permanentPrivateBrowsing) {
>        // Disable switch to tab autocompletion for private windows 
>        // (not for "Always use private browsing" mode)
> +      let value = gURLBar.getAttribute("autocompletesearchparam") || "";
> +      if (value.split(" ").indexOf("disable-switch-to-tab") == -1) {

it would be simpler to use !value.contains("disable-switch-to-tab"), the likely we might have other strings matching this is so low it's not even worth paying the cost of split.

::: toolkit/components/places/UnifiedComplete.js
@@ +523,5 @@
>                                        : Prefs.emptySearchDefaultBehavior;
> +
> +  let params = new Set(searchParam.split(" "));
> +  this._enableActions = params.has("enable-actions");
> +  this._disableSwitchToTab = params.has("disable-switch-to-tab");

Hm, if we'd have the patch from bug 530209, we could just remove the "openpage" behavior... Maybe worth filing a follow-up to investigate merging this with that.

what about this._switchToTabEnabled = this._enableActions && !params.has...

@@ +1155,5 @@
>      // If actions are enabled and the page is open, add only the switch-to-tab
>      // result.  Otherwise, add the normal result.
>      let url = escapedURL;
>      let action = null;
> +    if (this._enableActions && !this._disableSwitchToTab && openPageCount > 0) {

and here just check this._switchToTabEnabled && openPageCount

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +493,5 @@
>        fixupSearchText(this._originalSearchString.toLowerCase());
>  
> +    let params = new Set(aSearchParam.split(" "));
> +    this._enableActions = params.has("enable-actions");
> +    this._disableSwitchToTab = params.has("disable-switch-to-tab");

ditto
Attachment #8501921 - Flags: review?(mak77) → feedback+

Updated

3 years ago
QA Contact: alexandra.lucinet
Comment on attachment 8501921 [details] [diff] [review]
0001-Bug-1075450-Disable-only-switch-to-tab-instead-of-al.patch, v2

Review of attachment 8501921 [details] [diff] [review]:
-----------------------------------------------------------------

(Sorry I saw this so late - didn't get a needinfo)

I'd rather have a general "private-window" param added, as a more generic solution that can handle any other actions/behaviour that may be impacted by private windows in the future.
(In reply to Blair McBride [:Unfocused] from comment #9)
> I'd rather have a general "private-window" param added, as a more generic
> solution that can handle any other actions/behaviour that may be impacted by
> private windows in the future.

Though, "private" or "private-window" would lie, cause it's not set in permanent private browsing mode.  It's sort of risky to rely on a lie.
Flags: needinfo?(bmcbride)

Updated

3 years ago
Iteration: 35.3 → 36.1
Er, oh.

"disable-private-actions"?

Either way, something that disables things by trait, rather than disables one specific thing.
Flags: needinfo?(bmcbride)
(Assignee)

Comment 12

3 years ago
Created attachment 8506284 [details] [diff] [review]
0001-Bug-1075450-Disable-only-switch-to-tab-instead-of-al.patch, v3

Addressed all comments and added a test for "disable-switch-to-tab" and actions disabled. Do you think we still need a test with a real private window? I guess that would have to be a m-bc then?
Attachment #8501921 - Attachment is obsolete: true
Attachment #8506284 - Flags: review?(mak77)
A couple things:
1. we should use "disable-private-actions" as Blair suggested.
2. I think we should wait for bug 530209 at this point, it makes easier to fix this more properly.
3. I think the test might be fine. Surely a b-c test would involve more code and provide a better behavioral testing, but it would also much more expensive to make right (since it involves multiple windows and private-browsing)
Depends on: 530209
the truth is this doesn't test that private-browsing windows set the correct params, having a test that just checks that would basically complete the coverage, without crazy interaction with autocomplete popups.
So I wonder if we couldn't just touch browser_tabMatchesInAwesomebar_perwindowpb.js... does that test pass with your patch actually?
(Assignee)

Comment 15

3 years ago
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #13)
> 1. we should use "disable-private-actions" as Blair suggested.

Will do.

> 2. I think we should wait for bug 530209 at this point, it makes easier to
> fix this more properly.

SGTM.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #14)
> So I wonder if we couldn't just touch
> browser_tabMatchesInAwesomebar_perwindowpb.js... does that test pass with
> your patch actually?

I am sure we can adapt that... and no it doesn't pass. I don't quite understand why though, isn't that supposed to test behavior in normal non-private windows? The test name indicates otherwise...

Waiting for bug 530209 then to continue.
(In reply to Tim Taubert [:ttaubert] from comment #15)
> I am sure we can adapt that... and no it doesn't pass. I don't quite
> understand why though, isn't that supposed to test behavior in normal
> non-private windows? The test name indicates otherwise...

yeah, now I'm really confused about what that test is testing... its name doesn't make sense at all.
Geez, I'm glad I'm not the only one that's been confused by that test.

So I just tracked down what happened: 
* First we had browser_tabMatchesInAwesomebar.js
* That original test included global PB test coverage
* Thn we added support for per-window PB, but it was behind a compile time config, 
MOZ_PER_WINDOW_PRIVATE_BROWSING
* So bug 806708 duplicated that test, introducing browser_tabMatchesInAwesomebar_perwindowpb.js which *removed* all PB testing when per-window PB was enabled, because browser_bug816527.js tests that
* Then we removed the compile-time config, so per-window PB was always enabled
* Then browser_tabMatchesInAwesomebar.js was removed, because it tested global PB mode, not the new per-window PB

So now we have:
* browser_tabMatchesInAwesomebar_perwindowpb.js which is really browser_tabMatchesInAwesomebar.js
* browser_bug816527.js which is really browser_tabMatchesInAwesomebar_perwindowpb.js
Comment on attachment 8506284 [details] [diff] [review]
0001-Bug-1075450-Disable-only-switch-to-tab-instead-of-al.patch, v3

Thanks for investigating the tests madness!

OK, let's clarify status of the bug.
The approach is f+ but we need to address blair's comment, we'd like to wait for Alex patch (so we can use removeBehavior(openpage), and we need to clarify the test situation.

I think at this point it's worth to do the renaming that Blair implicitly suggested
* browser_tabMatchesInAwesomebar_perwindowpb.js to browser_tabMatchesInAwesomebar.js
* browser_bug816527.js to browser_tabMatchesInAwesomebar_perwindowpb.js

and we can try to add a test for this in the NEW browser_tabMatchesInAwesomebar_perwindowpb.js ?
Attachment #8506284 - Flags: review?(mak77) → feedback+

Updated

3 years ago
Iteration: 36.1 → ---

Updated

3 years ago
Iteration: --- → 36.3

Updated

3 years ago
QA Contact: alexandra.lucinet → andrei.vaida
(Assignee)

Comment 19

3 years ago
Created attachment 8526650 [details] [diff] [review]
0001-Bug-1075450-Disable-some-Awesomebar-actions-for-priv.patch, v4
Attachment #8506284 - Attachment is obsolete: true
Attachment #8526650 - Flags: review?(mak77)
(Assignee)

Comment 20

3 years ago
Comment on attachment 8526650 [details] [diff] [review]
0001-Bug-1075450-Disable-some-Awesomebar-actions-for-priv.patch, v4

Review of attachment 8526650 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_tabMatchesInAwesomebar_perwindowpb.js
@@ +91,5 @@
> +
> +            // Execute the selected action.
> +            controller.handleEnter(true);
> +          });
> +        });

These lines and above are the only real changes I made to browser_bug816527.js. All the other changes are just noise from renaming the tests :/
(Assignee)

Comment 21

3 years ago
Created attachment 8526668 [details] [diff] [review]
0001-Bug-1075450-Disable-some-Awesomebar-actions-for-priv.patch, v5

Patch without renaming tests.
Attachment #8526650 - Attachment is obsolete: true
Attachment #8526650 - Flags: review?(mak77)
Attachment #8526668 - Flags: review?(mak77)
(Assignee)

Comment 22

3 years ago
Created attachment 8526669 [details] [diff] [review]
0002-Bug-1075450-Rename-tests.patch

Separate patch to rename tests.
Attachment #8526669 - Flags: review?(mak77)
(Assignee)

Comment 23

3 years ago
Created attachment 8526680 [details] [diff] [review]
0002-Bug-1075450-Rename-tests-part-1-2.patch

VCS are stupid sometimes.
Attachment #8526669 - Attachment is obsolete: true
Attachment #8526669 - Flags: review?(mak77)
Attachment #8526680 - Flags: review?(mak77)
(Assignee)

Comment 24

3 years ago
Created attachment 8526681 [details] [diff] [review]
0003-Bug-1075450-Rename-tests-part-2-2.patch
Attachment #8526681 - Flags: review?(mak77)
(Assignee)

Comment 25

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=a8b17d17fbf0

Updated

3 years ago
Attachment #8526680 - Flags: review?(mak77) → review+

Updated

3 years ago
Attachment #8526681 - Flags: review?(mak77) → review+
Comment on attachment 8526668 [details] [diff] [review]
0001-Bug-1075450-Disable-some-Awesomebar-actions-for-priv.patch, v5

Review of attachment 8526668 [details] [diff] [review]:
-----------------------------------------------------------------

it looks good, not making cause you said still looking into the test.

::: browser/base/content/test/general/browser_bug816527.js
@@ +79,5 @@
> +        urlbar.value = testURL;
> +        controller.startSearch(testURL);
> +
> +        // Wait for the Awesomebar popup to appear.
> +        waitForAwesomebarPopup(aDestWindow, function () {

fwiw, we have promiseSearchComplete that maybe could help you here.. actually I just figured its name is not very nice cause "search" could also refer to the searchbar or fayt. but I'd not rename it here cause I don't want to bitrot Blair (hope those patches land soon...)
(we also have promisePopupShown, promisePopupHidden, and promisePopupEvent)

@@ +81,5 @@
> +
> +        // Wait for the Awesomebar popup to appear.
> +        waitForAwesomebarPopup(aDestWindow, function () {
> +          // Wait until there are at least two matches.
> +          waitForCondition(() => controller.matchCount > 1, function () {

I wonder if this could be used to make promiseSearchComplete reliable on linux... does it work for you there?

@@ +86,5 @@
> +            // Select the second match.
> +            // TODO Remove one of those when bug 1067903 lands and the value
> +            //      of the second entry starts with "moz-action:switchtab,".
> +            controller.handleKeyNavigation(KeyEvent.DOM_VK_DOWN);
> +            controller.handleKeyNavigation(KeyEvent.DOM_VK_DOWN);

you should notify Blair of this change or he could rely on a previous green try and get orange.
Attachment #8526668 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 27

3 years ago
Created attachment 8527284 [details] [diff] [review]
0001-Bug-1075450-Disable-some-Awesomebar-actions-for-priv.patch, v6

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #26)
> ::: browser/base/content/test/general/browser_bug816527.js
> @@ +79,5 @@
> > +        urlbar.value = testURL;
> > +        controller.startSearch(testURL);
> > +
> > +        // Wait for the Awesomebar popup to appear.
> > +        waitForAwesomebarPopup(aDestWindow, function () {
> 
> fwiw, we have promiseSearchComplete that maybe could help you here..
> actually I just figured its name is not very nice cause "search" could also
> refer to the searchbar or fayt. but I'd not rename it here cause I don't
> want to bitrot Blair (hope those patches land soon...)
> (we also have promisePopupShown, promisePopupHidden, and promisePopupEvent)

Ah, promisePopupShown() looks good, using that.

> @@ +81,5 @@
> > +
> > +        // Wait for the Awesomebar popup to appear.
> > +        waitForAwesomebarPopup(aDestWindow, function () {
> > +          // Wait until there are at least two matches.
> > +          waitForCondition(() => controller.matchCount > 1, function () {
> 
> I wonder if this could be used to make promiseSearchComplete reliable on
> linux... does it work for you there?

Yeah, my test seems to work fine on Linux. I can work on promiseSearchComplete() for bug 1073339 as a follow-up.

> @@ +86,5 @@
> > +            // Select the second match.
> > +            // TODO Remove one of those when bug 1067903 lands and the value
> > +            //      of the second entry starts with "moz-action:switchtab,".
> > +            controller.handleKeyNavigation(KeyEvent.DOM_VK_DOWN);
> > +            controller.handleKeyNavigation(KeyEvent.DOM_VK_DOWN);
> 
> you should notify Blair of this change or he could rely on a previous green
> try and get orange.

I can wait until bug 1067903 lands, hope that happens soon.
Attachment #8526668 - Attachment is obsolete: true
Attachment #8527284 - Flags: review?(mak77)
(Assignee)

Comment 28

3 years ago
Try run of latest patch:

https://tbpl.mozilla.org/?tree=Try&rev=bfc0adef87aa
(Assignee)

Updated

3 years ago
Blocks: 1073339
Depends on: 1067903
(Assignee)

Comment 29

3 years ago
Comment on attachment 8527284 [details] [diff] [review]
0001-Bug-1075450-Disable-some-Awesomebar-actions-for-priv.patch, v6

Will update the patch now that the default action is selected by default.
Attachment #8527284 - Flags: review?(mak77)
(Assignee)

Comment 30

3 years ago
Created attachment 8527564 [details] [diff] [review]
0001-Bug-1075450-Disable-some-Awesomebar-actions-for-priv.patch, v7

Simplified the test further.
Attachment #8527284 - Attachment is obsolete: true
Attachment #8527564 - Flags: review?(mak77)
(Assignee)

Comment 31

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=1e7f468f210d
Comment on attachment 8527564 [details] [diff] [review]
0001-Bug-1075450-Disable-some-Awesomebar-actions-for-priv.patch, v7

Review of attachment 8527564 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_bug816527.js
@@ +88,2 @@
>  
> +          // Wait until there are at least two matches.

this comment likely needs a brief update
Attachment #8527564 - Flags: review?(mak77) → review+
(Assignee)

Comment 33

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/a093db3222c6
https://hg.mozilla.org/integration/fx-team/rev/f7d199020d74
https://hg.mozilla.org/integration/fx-team/rev/8ec21ff65ce3

Updated

3 years ago
Iteration: 36.3 → 37.1
https://hg.mozilla.org/mozilla-central/rev/a093db3222c6
https://hg.mozilla.org/mozilla-central/rev/f7d199020d74
https://hg.mozilla.org/mozilla-central/rev/8ec21ff65ce3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Verified fixed on Nightly 37.0a1 (2014-12-01) and Aurora 36.0a2 (2014-12-01) using Windows 7 64-bit, Ubuntu 12.04 LTS 32-bit and Mac OS X 10.9.5 with the 'browser.urlbar.unifiedcomplete' pref set to true.
Status: RESOLVED → VERIFIED
status-firefox36: --- → verified
status-firefox37: --- → verified
You need to log in before you can comment on or make changes to this bug.