Closed Bug 1078718 Opened 5 years ago Closed 5 years ago

Incorrect URL screen results in "dead" tab

Categories

(Hello (Loop) :: Client, defect)

defect
Not set

Tracking

(firefox35 fixed, firefox36 fixed)

VERIFIED FIXED
mozilla36
Iteration:
36.1
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
Blocking Flags:
backlog Fx36+

People

(Reporter: drno, Unassigned)

References

Details

(Whiteboard: [standalone])

Attachments

(1 file, 2 obsolete files)

After clicking on the green "Start" button on a invalid URL like this https://call.mozilla.com/#call/joL2Z0NlHw1 I get an error page "Oops! Sorry, this URL is not available. ...".
If I try to fix the incorrect call URL parameter from joL2Z0NlHw1 to joL2Z0NlHw0 (just examples), nothing happens. Manipulating the URL parameters and hitting enter seems to do nothing.
You can only:
- close the tab and open a new one
- or reload the tab and then fix the URL

BTW manipulating the call URL parameter on this error page and then hitting the reload icon in the URL bar, results in reloading the page with the new parameter?!
Having talked to NiKo, we think the best thing to do for now would be to listen to the window's hashchange event, and when it fires, force a reload.

This saves us having from having to abort any states, and ensures we get a fresh set-up with the new token.
Blocks: 1079814
Attachment #8502498 - Flags: feedback?(nperriault)
Comment on attachment 8502498 [details] [diff] [review]
Force standalone app reload on hashchange event

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

To make all this a bit more reusable, I'd rather see this split in two simple mixins: 

- a UrlHashChangeMixin allowing the component it's applied to define a onUrlHashChange method, which would be called every time the url hash changes;
- a DocumentLocation mixin, exposing the locationReload method

That'd allow us to define use them that way:

var Component = React.createClass({
  mixins: [UrlHashChangeMixin, DocumentLocationMixin],
  
  onUrlHashChange: function() { // called by UrlHashChangeMixin
    this.locationReload();      // provided by DocumentLocationMixin
  }
});

What do you think?
Attachment #8502498 - Flags: feedback?(nperriault)
Attachment #8502498 - Attachment is obsolete: true
Attachment #8503068 - Flags: review?(nperriault)
Comment on attachment 8503068 [details] [diff] [review]
Force standalone app reload on hashchange event

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

Looks great.

::: browser/components/loop/content/shared/js/mixins.js
@@ +32,5 @@
> +   */
> +  var UrlHashChangeMixin = {
> +    _onUrlHashChange: function() {
> +      if (!this.onUrlHashChange) {
> +        throw new ReferenceError("onUrlHashChange callback is not defined");

Let's just fail silently.
Attachment #8503068 - Flags: review?(nperriault) → review+
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #5)
> ::: browser/components/loop/content/shared/js/mixins.js
> > +        throw new ReferenceError("onUrlHashChange callback is not defined");
> 
> Let's just fail silently.

Now I think about it, what we probably want here is to define propTypes for each mixins, and ask for the onUrlHashChange prop to be a required function.
Niko -- Is this ready to land (with a little clean up)?
backlog: --- → Fx36?
Flags: needinfo?(nperriault)
This can land with the comment addressed, yes. I'll update the patch.
Flags: needinfo?(nperriault)
Assignee: nobody → nperriault
Updated patch to address the previsou comment, and updated mixin tests to not use the real DOMWindow object.
Attachment #8503068 - Attachment is obsolete: true
Attachment #8510902 - Flags: review?(standard8)
Comment on attachment 8510902 [details] [diff] [review]
Force standalone app reload on hashchange event

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

Looks good. r=Standard8.

Note that we won't need this code for a long time, as bug 1046114 is changing the formats. However, its useful in the short term as it lets us fix the issue and not need to be dependent on when the configuration changes for the various servers are rolled out.
Attachment #8510902 - Flags: review?(standard8) → review+
https://hg.mozilla.org/integration/fx-team/rev/86a90b651868
backlog: Fx36? → Fx36+
Whiteboard: [standalone]
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/86a90b651868
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Nils, can you please verify this is fixed in tomorrow's Nightly?
Flags: qe-verify+
Flags: in-testsuite+
QA Contact: anthony.s.hughes → drno
Iteration: --- → 36.1
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #13)
> Nils, can you please verify this is fixed in tomorrow's Nightly?

Nils -- is this fixed for you?  (Thanks!)
Flags: needinfo?(drno)
Comment on attachment 8510902 [details] [diff] [review]
Force standalone app reload on hashchange event

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8510902 - Flags: approval-mozilla-aurora?
Attachment #8510902 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
It is not fixed for me. I still get exactly the behavior as before on 36.0a1 (2014-11-13) and 35.0a2 (2014-11-13).

But if my understanding is correct this can't get magically fixed in Firefox as we are talking about URL's and content getting delivered through the Loop content server, or?
So I would need to either wait for the next production deployment which includes this fix, or ask the services guys for a deployment which includes this fix for test verification.
Flags: needinfo?(drno)
Hi Nils, I'm sorry that I wasn't clearer.  This is standalone.  So you would need to test it against a dev server that has this fix -- or test it when the next standalone is deployed to prod (which is scheduled to be pushed out tomorrow).  At this point I'll ping you right after the new standalone is out.
I can test against dev if that helps and someone lets me know the URL for dev.
Verified fixed on the standalone page in FF 35b5, Chrome.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.