1104165 - "Switch to Tab" doesn't seem to work for Google Calendar

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Load Google Calendar.  Might need to log in.  In my case this lands me at https://www.google.com/calendar/render#g|week-2+22903+22909+22904

2)  Open a new window.

3)  Type "% calendar" in the URL bar.

4)  Arrow one down and pick the "Switch to Tab" option in the dropdown.

EXPECTED RESULTS: Calendar window/tab is focused

ACTUAL RESULTS: Nothing happens.

Same thing if in step 2 I open a new tab instead of a new window.

This generally works on other sites, so something is special about Google Calendar here....

Comment 1

[Boris Zbarsky [:bzbarsky]]

So when we're inside switchIfURIInWindow, I see the following:

  ignoreFragment: undefined
  browser.currentURI.spec:
    https://www.google.com/calendar/render?pli=1#g%7Cweek-2+22903+22909+22904
  aURI.spec: 
    https://www.google.com/calendar/render?pli=1#g|Cweek-2+22903+22909+22904

So the question is, why the "%7C" vs "|" difference?

Well, if I load this URI:

  http://web.mit.edu/

and then run this in the console:

  history.pushState(null, "aa", "#%7C")

then I get the same difference in behavior.

In any case, it seems like the issue is that we compare aURI to browser.currentURI, but the former is some sort of prettified version which doesn't actually compare equal in this case.  We should presumably be passing in non-prettified versions here...

Comment 2

[:Gijs (out for now; he/him)]

Hrmpf. This gets passed from the autocomplete dropdown entry, and I don't know where those are created from... Unfortunately, because those are magical action URLs with a nested URI, they are doomed to be strings. I don't know if this is easily fixable without running into the behaviour from bug 483304 and friends. Poking some more.

Comment 3

[:Gijs (out for now; he/him)]

So the unifiedcomplete code (which generates this URL) is sane and keeps the escaping - in particular, the code here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#550 leaves the escaping well alone.

Then I tried the other end, but trimURL (utilityOverlay.js) is also not at fault - its input is already broken. Same for parseActionUrl here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#741 .

No, the issue is in the middle. When the autocomplete binding gets the enter keypress, the url bar binding's setTextValue method gets invoked:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#713

and that calls losslessDecodeURI:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2347

which decodes the pipe here.

Now, I don't know whether we should just update that pile of replace statements... or if there's some reason that's a bad idea. Marco/Dão, opinions? Can we fix this somewhere else in this path?

Leaving in tabbed browser for now since this impacts switch-to-tab more than anything else, but there are only two places that call losslessDecodeURI, which are URLBarSetURI and the one linked above, so if we decide we need to update that, I guess ::Location Bar is more appropriate.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Seems to me like using the output of losslessDecodeURI and then comparing it to the original URI is just going to fail when the URI contains any of those chars, right?  It just happens that this URI does, but there's more stuff here than just '|' as far as I can tell.  Presumably the losslessDecodeURI output is what's being shown to the user, so we want to keep doing that, but we should be storing the actual URI as well, and using that when the dropdown option is selected.

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #4)
> Seems to me like using the output of losslessDecodeURI and then comparing it
> to the original URI is just going to fail when the URI contains any of those
> chars, right?  It just happens that this URI does, but there's more stuff
> here than just '|' as far as I can tell.  Presumably the losslessDecodeURI
> output is what's being shown to the user, so we want to keep doing that, but
> we should be storing the actual URI as well, and using that when the
> dropdown option is selected.

The problem is that how this currently works is, selecting the element from the dropdown puts the action URL in the URL bar using the appropriate magic (including losslessDecodeURI), and then navigates to it, which is what triggers the tab switch. The putting the action URL in the URL bar is what munges the URI here; there isn't really a mechanism in place to keep the actual URI "around" for switching. We could try and hack one in, but I'm not sure how fragile it'd be.

Comment 6

[:Gijs (out for now; he/him)]

IOW, the "what's being shown to the user" and "what we navigate to" are the same, and because of how the code goes via the URL bar instead of just doing something based on the relevant autocomplete entry, having additional 'real' URL data to use once we get to switchToTabHavingURI is nontrivial, AFAICT...

The quickest fix I can think of would be to just ladle over another layer of encoding/decoding to avoid this happening for switch-to-tab URIs, but I don't know that that won't break in other craptacular ways. :-\

Comment 7

[Boris Zbarsky [:bzbarsky]]

> and then navigates to it

Er... Except that we're not navigating; we're doing a tab switch, no?  I mean, the URL is shown in the URL bar, but with a clear "switch to tab:" action.

> The putting the action URL in the URL bar is what munges the URI here

OK, so we need to stop doing that or adding more info in the tab switch case or something...

> but I don't know that that won't break in other craptacular ways. :-\

Indeed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So the other option is to make the comparisons in switchIfURIInWindow more lenient somehow, I guess...

Comment 9

[:Gijs (out for now; he/him)]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #7)
> > and then navigates to it
> 
> Er... Except that we're not navigating; we're doing a tab switch, no?  I
> mean, the URL is shown in the URL bar, but with a clear "switch to tab:"
> action.

From the perspective of the docshell, we don't navigate. From the perspective of the location bar, we call handleCommand:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#276

which deals with the magic URI, and so I oversimplified that to 'navigate'. We tell the location bar "go to this URI", which then happens to be a switch-to-tab URI.

> > The putting the action URL in the URL bar is what munges the URI here
> 
> OK, so we need to stop doing that or adding more info in the tab switch case
> or something...

Yup. But we prefill the URL if you arrow down (for 'normal' and switch-to-tab and 'search' actions alike) so that enter/return then 'does the right thing'. Last time I looked, the hover+click path to autocomplete does roughly the same. I doubt that we'd want to be changing that, and I suspect it won't be trivial to add arbitrary data. I mean, we could always build an in-memory map with keys "things in the URL bar" to values "things we actually mean". Just feels super fragile and/or ugly.

Comment 10

[Marco Bonardo [:mak]]

So, what about we change the "registered" open url?
See registerOpenPage and unregisterOpenPage... we could maybe register the non-prettified  uri, and that is what we'd compare currentURI with.

Comment 11

[Marco Bonardo [:mak]]

basically here http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#807

Comment 12

[:Gijs (out for now; he/him)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #10)
> So, what about we change the "registered" open url?
> See registerOpenPage and unregisterOpenPage... we could maybe register the
> non-prettified  uri, and that is what we'd compare currentURI with.

But we do register the non-prettified URI. The problem is that the location bar munges it when we move the value from the autocomplete result to the location bar as part of the switch-to-tab URI.

Comment 13

[Marco Bonardo [:mak]]

hrm, right...

Looks like we need to store the original value before it gets prettified, so we can reuse it for switch to tab. We already separate the input value from the completion value, though the .value, .textValue, ._value, inputField.value situation is sort of a nightmare...

The ideal fix would probably retain the unprettified url in _value.

I wonder if we could move losslessDecodeURI into onBeforeValueSet (after setting _value) and in onBeforeValueGet, so that _value stays unmodified. that is what we usually parse for action urls.

Blair is the best person to evaluate this since he should have the freshest memory about the various value getters/setters in urlbarBindings.

Comment 14

[Marco Bonardo [:mak]]

Attachment: 1104165.diff

So, I'm basically suggesting we do this.

The 2 problems are:
1. we need a test and I currently don't have the time to write one (but we could schedule this for the next iteration)
2. I'm not sure this is regression-safe, Blair might probably evaluate my approach from this point of view

Comment 15

[:Gijs (out for now; he/him)]

Clever and slightly evil, I think I like it. I can sign up to write a regression test for this tomor... I mean post-sleep. We already have switch-to-tab tests, so in principle (hah!) this shouldn't be too hard...

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8529354 [details] [diff] [review]
1104165.diff

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

Ugh. Just... ugh.

But yea, this works. Certainly the easiest thing, without needing to rewrite even further how the urlbar handles values (I feel like bug 1067903 only half did that, and you know of the troubles I had there).

Comment 17

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #15)
> Clever and slightly evil, I think I like it. I can sign up to write a
> regression test for this tomor... I mean post-sleep. We already have
> switch-to-tab tests, so in principle (hah!) this shouldn't be too hard...

(Sorry, I'm going to have to bail on this promise - let's make sure we schedule something for the next iteration.)

Comment 18

[Marco Bonardo [:mak]]

I'll try to work on this for the next iteration.

Comment 19

[Marco Bonardo [:mak]]

added to suggestions for 38.1

Comment 20

[Marco Bonardo [:mak]]

Attachment: patch v2

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8553174 [details] [diff] [review]
patch v2

sigh, looks like browser_urlHighlight.js, browser_urlbarAutoFillTrimURLs.js and browser_urlbarTrimURLs.js are unhappy
looks like an UC/lc thing

Comment 22

[Marco Bonardo [:mak]]

the problem is the makeURI call, when value is set, the uri gets normalized by nsIURI... but actually, there's no real reason for losslessDecodeURI to take an nsIURI, it could act on an href too.

Comment 23

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #22)
> but actually, there's no real reason for losslessDecodeURI to
> take an nsIURI, it could act on an href too.

The reason is that only an URI should be encoded in a way that it makes sense to decode it like that.

I'm not sure what you mean by "href" in the context of your patch. We shouldn't blindly "decode" strings that are set through gURLBar.value.

Comment 24

[Marco Bonardo [:mak]]

I think in all of the cases where our code sets .value, we want it to be decoded, provided it's an url. So, I think it makes sense for the value getter to do the decodeURI, as suggested.
The problem here is that tests expect setting .value and getting .textValue to return the unmodified value, but what they pass to value is not a normalized nsIURI spec (for example we do .value = "http://[a:b:C:d:e:f:g:h]" we get back "http://[a:b:c:d:e:f:g:h]", for "http://" we get back "http:///"). I'd be fine changing the tests, but I'd first like to get your opinion.
More specifically I'm not sure why browser_urlHighlight.js cares about setting strings like [1:2:a:B:c:D:e:F] rather than the normalized [1:2:a:b:c:d:e:f]?

The alternative could be to make losslessDecodeURI take either nsIURI or a string, if it gets a string it tries to makeURI it (throws if that fails) but it will still decode the original string. But I think it would be better to fix the tests.

Comment 25

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #24)
> So, I think it makes sense for the value
> _setter_ to do the decodeURI, as suggested.

sorry, typo there.

Comment 26

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #24)
> I think in all of the cases where our code sets .value, we want it to be
> decoded, provided it's an url.

The assumption that it's an URL simply isn't sane. Anyone (including add-ons, not just our code) can set any value they want and then submit it or just leave it there for the user to edit it.

> More specifically I'm not sure why browser_urlHighlight.js cares about setting strings like
> [1:2:a:B:c:D:e:F] rather than the normalized [1:2:a:b:c:d:e:f]?

Because it wants such values to be highlighted correctly (and implicitly, the expectation is that the value setter doesn't mess with them).

Comment 27

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [:dao] from comment #26)
> (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #24)
> > I think in all of the cases where our code sets .value, we want it to be
> > decoded, provided it's an url.
> 
> The assumption that it's an URL simply isn't sane. Anyone (including
> add-ons, not just our code) can set any value they want and then submit it
> or just leave it there for the user to edit it.

wait, if it's an url we decode it, otherwise we let it go through as-is.
It's not much different than today. The only difference is that today we decodeURI if you set textValue to an url and then internally it sets .value to the decoded value. But if you set value to an url, it's not decoded.
With the change we'd decode in the .value setter, so setting either value or textValue will set a decoded value.
non-uri values are pass-through as usual.

> > More specifically I'm not sure why browser_urlHighlight.js cares about setting strings like
> > [1:2:a:B:c:D:e:F] rather than the normalized [1:2:a:b:c:d:e:f]?
> 
> Because it wants such values to be highlighted correctly (and implicitly,
> the expectation is that the value setter doesn't mess with them).

I think that expectation is sort-of bogus. The hilight happens once the url has been confirmed as good and the page is loading. But at that point the url has already been normalized and what the user sees is not "gOoGle.com" but "google.com".
Sounds like the test is explicitly testing that setting .value and reading .textValue gets back the same, and I'm not sure it's a useful feature.

Comment 28

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #27)
> > > More specifically I'm not sure why browser_urlHighlight.js cares about setting strings like
> > > [1:2:a:B:c:D:e:F] rather than the normalized [1:2:a:b:c:d:e:f]?
> > 
> > Because it wants such values to be highlighted correctly (and implicitly,
> > the expectation is that the value setter doesn't mess with them).
> 
> I think that expectation is sort-of bogus. The hilight happens once the url
> has been confirmed as good and the page is loading. But at that point the
> url has already been normalized and what the user sees is not "gOoGle.com"
> but "google.com".

That's not really accurate... for instance, you can enter any URL in the location bar and remove focus without loading anything...

Comment 29

[Marco Bonardo [:mak]]

Attachment: patch v3

This is the less invasive patch I may think of, it only fixes this specific bug without touching anything else. Any other kind of change I tried, touches some of the current behaviors and ends up being hairy. Since I have higher priorities and I'm basically out of ideas and time, I might have to drop this bug if this is unacceptable.

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 8595808 [details] [diff] [review]
patch v3

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -2413,16 +2413,24 @@ function losslessDecodeURI(aURI) {
>                         encodeURIComponent);
> 
>   // Encode default ignorable characters (bug 546013)
>   // except ZWNJ (U+200C) and ZWJ (U+200D) (bug 582186).
>   // This includes all bidirectional formatting characters.
>   // (RFC 3987 sections 3.2 and 4.1 paragraph 6)
>   value = value.replace(/[\u00ad\u034f\u061c\u115f-\u1160\u17b4-\u17b5\u180b-\u180d\u200b\u200e-\u200f\u202a-\u202e\u2060-\u206f\u3164\ufe00-\ufe0f\ufeff\uffa0\ufff0-\ufff8]|\ud834[\udd73-\udd7a]|[\udb40-\udb43][\udc00-\udfff]/g,
>                         encodeURIComponent);
>+
>+  // Store the encoded URL in action URLs.
>+  let action = gURLBar.parseActionUrl(value);
>+  if (action && action.params.url) {
>+    action.params.encodedUrl = gURLBar.parseActionUrl(aURI.spec).params.url;
>+    value = "moz-action:" + action.type + "," + JSON.stringify(action.params);
>+  }

I don't think losslessDecodeURI is the right place to do that, as I don't think losslessDecodeURI should get the whole moz-action URI in the first place. We really only want to decode what we show to the user, and that's action.params.url in this case. Can the textValue setter take care of this?

Comment 31

[Marco Bonardo [:mak]]

yes and no. It can take care of this, but it's not just params.url, in other cases like action = searchengine, we build an url using getSubmissionURI. I might see if in this latter case I can add a losslessDecodeURI at the url generation point.

losslessDecodeURI is also used by URLBarSetURI, though I think we can never get a moz-action url there, since URLBarSetURI is only invoked with an argument only from onLocationChange? Maybe it should reportError if it gets a moz-action URI for future safety?

Comment 32

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [::mak] from comment #31)
> losslessDecodeURI is also used by URLBarSetURI, though I think we can never
> get a moz-action url there, since URLBarSetURI is only invoked with an
> argument only from onLocationChange?

yes

> Maybe it should reportError if it gets a moz-action URI for future safety?

Or maybe losslessDecodeURI should report that error...

Comment 33

[Marco Bonardo [:mak]]

Attachment: patch v4

Ok, let's see if Try is happy with this one
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ac7f9514345

Comment 34

[Marco Bonardo [:mak]]

Attachment: patch v5

This one is passing all the tests and it's still well contained...

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 8596159 [details] [diff] [review]
patch v5

> function losslessDecodeURI(aURI) {
>   var value = aURI.spec;
>+  if (value.startsWith("moz-action:"))
>+    throw new Error("losslessDecodeURI should never get a moz-action URI");

how about:

>function losslessDecodeURI(aURI) {
>  if (aURI.schemeIs("moz-action"))
>    throw new Error("losslessDecodeURI should never get a moz-action URI");
>
>  var value = aURI.spec;


>           let matchLastLocationChange = true;
>           if (action) {
>             if (action.type == "switchtab") {
>               url = action.params.url;
>               if (this.hasAttribute("actiontype")) {
>                 this.handleRevert();
>                 let prevTab = gBrowser.selectedTab;
>-                if (switchToTabHavingURI(url) &&
>+                if (switchToTabHavingURI(action.params.encodedUrl || action.params.url) &&

if (switchToTabHavingURI(action.params.encodedUrl || url) &&

>+          if (uri) {
>+            // Store the encoded URL in action URLs.
>+            let action = this._parseActionUrl(val);
>+            if (action) {
>+              if (action.params.url) {
>+                action.params.encodedUrl = action.params.url;
>+                action.params.url = losslessDecodeURI(makeURI(action.params.url));
>+                val = "moz-action:" + action.type + "," + JSON.stringify(action.params);
>+              }
>+            } else {
>+              val = losslessDecodeURI(uri);
>+            }
>+          }

Please move the above comment to where it's relevant, e.g. inside 'if (action) {' or inside 'if (action.params.url) {' even.

I'd prefer params.originalUrl over params.encodedUrl.

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2ee8f0b9994d

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ee8f0b9994d

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a37cc5ecc92d

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a37cc5ecc92d

Comment 41

[Cornel Ionce [:noni] [Hubs QA]]

Verified issue as fixed on Mac OS X 10.9.5, using latest Nightly, build ID: 20150426030248

Comment 42

[Nicolas Barbulesco]

See also: issue bug 983809.