Closed Bug 1024133 Opened 5 years ago Closed 5 years ago

Switch to tab magic URL leakage

Categories

(Firefox :: Address Bar, defect)

33 Branch
x86
All
defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 33
Iteration:
33.3
Tracking Status
firefox30 --- unaffected
firefox31 + wontfix
firefox32 + verified
firefox33 + verified
firefox-esr24 --- unaffected

People

(Reporter: rnewman, Assigned: mak)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

* Open a tab.
* Type part of the tab name in a URL bar, such that the first result is Switch To Tab.
* Hit the down arrow to select it.
* Hold shift and press the left arrow.

You'll be left with text in the location bar:

moz-action:switchtab,https://bugzilla.mozilla.org/show_bug.cgi?id=965377
sigh, so many edge cases.
Do you know if this is a recent regression?
I didn't try to find a regression range, but based on my keyboarding habits it should be recent. (This is how I grab bug numbers.)
Regression window(fx)
Good:
https://hg.mozilla.org/integration/fx-team/rev/875df781fa8b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140604005825
Bad:
https://hg.mozilla.org/integration/fx-team/rev/035c1df29690
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140604052419
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=875df781fa8b&tochange=035c1df29690

Regressed by:
035c1df29690	Marco Bonardo — Bug 1003461 - Shift + Switch to Tab no longer respected. r=mano
OS: Mac OS X → All
Basic feature, tracking.
Flags: firefox-backlog+
Marco, could you have a look here? Thanks
Flags: needinfo?(mak77)
making a test should be easy (just matter of copying the test for bug 1003461 with slightly changes), let's see the patch.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
fwiw, I think this regression is much more edge-case than bug 1003461 and thus lower priority, I don't think it's worth a backout of bug 1003461.
Hi Marco, are you taking Bug 1024133 during iteration 33.2?
Flags: needinfo?(mak77)
(In reply to Marco Mucci [:MarcoM] from comment #8)
> Hi Marco, are you taking Bug 1024133 during iteration 33.2?

yes, please add it, I'm setting it to 5 points.
Points: --- → 5
Flags: needinfo?(mak77)
Added to iteration 33.2
Iteration: --- → 33.2
QA Whiteboard: [qa?]
Duplicate of this bug: 1030447
(In reply to Marco Bonardo [:mak] from comment #7)
> fwiw, I think this regression is much more edge-case than bug 1003461 and
> thus lower priority, I don't think it's worth a backout of bug 1003461.

FWIW, I didn't even know shift-enter was meant to do something different when arrowing through the autocomplete list, but I do regularly alter URLs from the autocomplete list, which causes this to show up... so I'm not sure I agree.
QA Whiteboard: [qa?] → [qa+]
QA Contact: catalin.varga
(In reply to :Gijs Kruitbosch from comment #12)
> but I do regularly alter URLs
> from the autocomplete list

I don't think you regularly edit switch to tab urls... I'd agree if it would be for any url. btw it'definitely something to fix, I'm just saying that a non-working switch to tab override is imo worse.
Iteration: 33.2 → 33.3
Attached patch patch v1 (obsolete) — Splinter Review
tentatively asking Mano, may forward in case he's busy.
Attachment #8458002 - Flags: review?(mano)
Flags: in-testsuite?
Comment on attachment 8458002 [details] [diff] [review]
patch v1

nevermind, seems to regress the previous bug...
Attachment #8458002 - Flags: review?(mano)
Attached patch patch v2Splinter Review
This is a safer approach for bug 1003461 that doesn't regress this use-case. both old and new test pass locally, plus some manual testing.

While there I also took care of the old bogus override behavior, where keeping shift pressed was disabling switch to tab randomly. it was due to using a counter to handle multiple override keys, but keydown happens more than once when keeping a key pressed. A Set imo is a much more robust solution to keep the buttons status.
Attachment #8458002 - Attachment is obsolete: true
Attachment #8458242 - Flags: review?(mano)
Comment on attachment 8458242 [details] [diff] [review]
patch v2

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

::: browser/base/content/test/general/browser_bug1024133-switchtab-override-keynav.js
@@ +20,5 @@
> +    } catch(ex) { /* tabs may have already been closed in case of failure */ }
> +  });
> +
> +  info("Wait for autocomplete")
> +  deferred = Promise.defer();

I tend to prefer a new variable, or let-scoped variables.

@@ +49,5 @@
> +  EventUtils.synthesizeKey("VK_SHIFT" , { type: "keyup" });
> +
> +  ok(!/moz-action:switchtab/.test(gURLBar.inputField.value), "switch to tab should be hidden");
> +
> +  yield promiseClearHistory();

shouldn't this be part of the cleanup function?

::: browser/base/content/urlbarBindings.xml
@@ +150,4 @@
>              returnValue = action.param;
> +          }
> +
> +          // Set the actiontype only if not overriding actions.

"if there are no"?

@@ +150,5 @@
>              returnValue = action.param;
> +          }
> +
> +          // Set the actiontype only if not overriding actions.
> +          if (action && this._noActionsKeys.size == 0) {

if (action) {
 ...
  if (this._noActionsKeys.size == 0) {
...
Attachment #8458242 - Flags: review?(mano) → review+
(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #18)
> > +  yield promiseClearHistory();
> 
> shouldn't this be part of the cleanup function?

registerCleanupFunction doesn't support async methods (unless something changed  recently)

> ::: browser/base/content/urlbarBindings.xml
> @@ +150,4 @@
> >              returnValue = action.param;
> > +          }
> > +
> > +          // Set the actiontype only if not overriding actions.
> 
> "if there are no"?

the fact is there are actions and we are not overriding them (through noAction keys)... I know the wording sucks a bit here, but "there are no actions" would be wrong.

> @@ +150,5 @@
> >              returnValue = action.param;
> > +          }
> > +
> > +          // Set the actiontype only if not overriding actions.
> > +          if (action && this._noActionsKeys.size == 0) {
> 
> if (action) {
>  ...
>   if (this._noActionsKeys.size == 0) {
> ...

the problem is that I must apply the else both if !action and if action && it's overridden. If i'd wrap everything into if (action) I should then duplicate the else or something fancy...
with fixed comments
https://hg.mozilla.org/integration/fx-team/rev/0047eb7e9292
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → Firefox 33
https://hg.mozilla.org/mozilla-central/rev/0047eb7e9292
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Verified as fixed using the following environment:

FF 33 Nightly
Build Id:20140720030203
OS: Win 7 x64, Ubuntu 13.04 x64, Mac Os X 10.7.5
QA Whiteboard: [qa+] → [qa!]
Comment on attachment 8458242 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 1003461 (regression from bug 754265)
[User impact if declined]: selecting text with shift on a switch to tab entry also selects special "switchtab" text action that should not exist
[Describe test coverage new/current, TBPL]: unit test
[Risks and why]: This is a regression in keyboard selection of the awesomebar, might be quite annoying for some users. low risk thanks to tests covering both regressions and small patch
[String/UUID change made/needed]: none
Attachment #8458242 - Flags: approval-mozilla-beta?
Attachment #8458242 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa!] → [qa+]
Verified as fixed using Firefox 32 beta 4 20140804164216 under Win 7 64-bit, Ubuntu 12.10 32-bit and Mac OSX 10.9.4.

"Switch to Tab: <url>" remains on the Location Bar until Enter key is pressed.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Blocks: 1025012
You need to log in before you can comment on or make changes to this bug.