Closed Bug 1104165 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

x86
macOS
defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox40 --- verified

People

(Reporter: bzbarsky, Assigned: mak)

References

Details

Attachments

(1 file, 4 obsolete files)

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....
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...
Flags: needinfo?(gijskruitbosch+bugs)
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.
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.
Flags: needinfo?(mak77)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dao)
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.
(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.
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. :-\
> 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.
So the other option is to make the comparisons in switchIfURIInWindow more lenient somehow, I guess...
(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.
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.
Flags: needinfo?(mak77)
(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.
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.
Flags: needinfo?(bmcbride)
Attached patch 1104165.diff (obsolete) — Splinter Review
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
Attachment #8529354 - Flags: feedback?(bmcbride)
Flags: needinfo?(dao)
Flags: needinfo?(bmcbride)
Blocks: 1071461
Points: --- → 3
Flags: qe-verify+
Flags: in-testsuite?
Flags: firefox-backlog+
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 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).
Attachment #8529354 - Flags: feedback?(bmcbride) → feedback+
(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.)
I'll try to work on this for the next iteration.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
added to suggestions for 38.1
Iteration: --- → 38.1 - 26 Jan
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8529354 - Attachment is obsolete: true
Attachment #8553174 - Flags: review?(bmcbride)
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
Attachment #8553174 - Flags: review?(bmcbride)
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.
(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.
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.
(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.
(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).
(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.
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
(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...
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #8553174 - Attachment is obsolete: true
Attachment #8595808 - Flags: review?(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?
Attachment #8595808 - Flags: review?(dao)
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?
(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...
Attached patch patch v4 (obsolete) — Splinter Review
Ok, let's see if Try is happy with this one
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ac7f9514345
Attachment #8595808 - Attachment is obsolete: true
Attached patch patch v5Splinter Review
This one is passing all the tests and it's still well contained...
Attachment #8595929 - Attachment is obsolete: true
Attachment #8596159 - Flags: review?(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.
Attachment #8596159 - Flags: review?(dao) → review+
https://hg.mozilla.org/mozilla-central/rev/2ee8f0b9994d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Duplicate of this bug: 983809
QA Contact: cornel.ionce
Verified issue as fixed on Mac OS X 10.9.5, using latest Nightly, build ID: 20150426030248
Status: RESOLVED → VERIFIED
Blocks: 983809
See also: issue bug 983809.
Duplicate of this bug: 983809
You need to log in before you can comment on or make changes to this bug.