957052 - Move remaining event handlers in mobile/android/chrome/content/aboutFeedback.xhtml (URL=about:feedback) to aboutFeedback.js

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Frederik Braun [:freddy]]

As Margaret pointed out, the event handlers are missing in the patch for bug 948897.
Would you be able to take this one too, Neil?

Comment 1

[Neil Bleasdale [:nbleasdale]]

Yep, can do.

Comment 2

[Neil Bleasdale [:nbleasdale]]

Attachment: shifted event handlers present in xhtml into the js file.

Comment 3

[:Margaret Leibovic]

Comment on attachment 8356842 [details] [diff] [review]
shifted event handlers present in xhtml into the js file.

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

On the right track! See my comments below.

::: mobile/android/chrome/content/aboutFeedback.js
@@ +8,5 @@
>  let Ci = Components.interfaces;
>  let Cu = Components.utils;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
> +initEventHandlers();

This isn't a very robust solution, since it depends on this script being loaded when the DOM is already ready. I would prefer you call initEventHandlers as a callback for a DOMContentLoaded event listener.

@@ +11,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
> +initEventHandlers();
> +
> +function initEventHandlers() {
> +	// maybe later

2-space indentation, please.

@@ +13,5 @@
> +
> +function initEventHandlers() {
> +	// maybe later
> +	let anchors = document.querySelectorAll(".maybe_later");
> +	for(var anchor of anchors) {

You should use let instead of var here.

@@ +17,5 @@
> +	for(var anchor of anchors) {
> +		anchor.addEventListener("click", maybeLater, false);
> +	}
> +	// happy/sad links
> +	document.querySelector("#happy_link").addEventListener(

Instead of querySelector, you should just use getElementById.

@@ +21,5 @@
> +	document.querySelector("#happy_link").addEventListener(
> +		"click", function(evt) {
> +			switch(evt.target.id) {
> +				case "happy_link":
> +					switchSection('happy');

Nit: Use double quotes for strings.

@@ +23,5 @@
> +			switch(evt.target.id) {
> +				case "happy_link":
> +					switchSection('happy');
> +					break;
> +				case "sad_link":

This callback is only added to the #happy_link element, so this case will never be reached. You could just include switchSection("happy") directly inside the body of the callback function.

@@ +31,5 @@
> +		}, false);
> +	document.querySelector("#sad_link").addEventListener(
> +		"click", function(evt) {
> +			switch(evt.target.id) {
> +				case "happy_link":

Same comment applies here.

@@ +41,5 @@
> +			}
> +		}, false);
> +
> +	// body onload/unload
> +	window.addEventListener("load", init, false);

I now realize that instead of making this new initEventHandlers function, you should just call init on DOMContentLoaded, and you can move all this new code into the init function (with the exception of this line here).

@@ +46,5 @@
> +	window.addEventListener("unload", uninit, false);
> +
> +	// play store link
> +	document.querySelector("#open_play_store").addEventListener(
> +		"click", openPlayStore, false);

You don't need to wrap longish lines like this, we don't follow a strict 80-character line limit (in fact, we prefer things to be on the same line as long as it's not too long, usually 100 characters).

@@ +54,5 @@
> +		}, false);
> +	// no thanks (exit)
> +	document.querySelector("#a_close").addEventListener(
> +		"click", function(evt) {
> +			window.close();

I think you should be able to just pass in window.close directly as the callback function here, rather than wrapping it in an anonymous function.

::: mobile/android/chrome/content/aboutFeedback.xhtml
@@ +26,5 @@
>  
>    <section id="intro" active="true">
>      <h1 class="header">&intro.header;</h1>
>      <div class="message">&intro.message;</div>
> +    <div id="happy_link" class="link-box" >

In this file we're already using hyphens instead of underscores for identifiers, so let's continue to do that here with these new ids/class names.

Also nit: Omit the space before the >.

@@ +49,5 @@
>        <a>&happy.ratingLink;</a>
>      </div>
>      <div class="bottom-links">
> +      <a class="maybe_later">&happy.maybeLater2;</a>
> +      <a id="a_close">&happy.noThanks;</a>

I think id="close" is fine here (there's no other element that has anything to do with closing).

Comment 4

[Neil Bleasdale [:nbleasdale]]

Attachment: bug957052.patch moved event handlers from xhtml to js.

Thanks for the feedback. Take two... 

The one change I couldn't change was to lose the anonymous function calling window.close(). When I tried document.getElementById("close").addEventListener("click", window.close, false); I got the following in the logcat: 
[JavaScript Error: "TypeError: 'close' called on an object that does not implement interface Window."]

Comment 5

[Frederik Braun [:freddy]]

This is a tricky case of DOM Clobbering: Given an element with the ID |foo|, the browser will automatically create |window.foo| (and, since |window| is the global scope for all objects, also just |foo| is enough) for you, which then refers to the element.

So with the |<a id="close">| element, you have basically overwritten the window.close method with a reference to the element. I recommend changing the ID back to something like closeButton or btnClose. Then |window.close| will work again

Comment 6

[Neil Bleasdale [:nbleasdale]]

Hah, that makes perfect sense. Thanks Frederik. I'll fix this when I get in from work this evening.

Comment 7

[Neil Bleasdale [:nbleasdale]]

Hmm, interesting. I had tried your suggestions Frederik, changing the id to something different like 'no-thanks' and using document.getElementById("no-thanks").addEventListener("click", window.close, false);

However, this results in exactly the same TypeError... any other suggestions?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to Frederik Braun [:freddyb] from comment #5)
> This is a tricky case of DOM Clobbering: Given an element with the ID |foo|,
> the browser will automatically create |window.foo|

Not if there's already a window.foo, though, so that doesn't explain the failure.

The issue with the snippet in comment 4 is that event listeners are invoked with their "this" as the event target, and window.close doesn't like that. So you should keep the anonymous wrapper function (or bind window.close to window, but that's a bit uglier IMO).

Comment 9

[:Margaret Leibovic]

Comment on attachment 8357301 [details] [diff] [review]
bug957052.patch moved event handlers from xhtml to js.

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

Looking better, I just have some more comments to address before we land this.

::: mobile/android/chrome/content/aboutFeedback.js
@@ +8,5 @@
>  let Ci = Components.interfaces;
>  let Cu = Components.utils;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
> +window.addEventListener("DOMContentLoaded", init, false);

You should add this listener on the document, not the window.

@@ +20,5 @@
>  }
>  
>  function init() {
> +  // setup event handlers
> +	// maybe later

You can get rid of these comments, since the classname/ids make it clear what's going on.

@@ +29,5 @@
> +	// happy/sad links
> +	document.getElementById("happy-link").addEventListener(
> +		"click", function(evt) {
> +      switchSection("happy");
> +		}, false);

To make this easier to read please format the addEventListener calls like this:

document.getElementById("happy-link").addEventListener("click", function(evt) {
  switchSection("happy");
}, false);

@@ +42,5 @@
> +	// play store link
> +	document.getElementById("open-play-store").addEventListener("click", openPlayStore, false);
> +	// form submission
> +	document.forms[0].addEventListener("submit", function(evt) {
> +			sendFeedback(evt);

Instead of passing along the event like this, just pass sendFeedback directly as the callback.

@@ +48,5 @@
> +	// no thanks (exit)
> +	document.getElementById("close").addEventListener("click", function(evt) {
> +      window.close();
> +    } , false);
> +    

Please fix the indentation here so that you use 2-space indentation everywhere. Also, please remove this trailing whitespace.

Comment 10

[Neil Bleasdale [:nbleasdale]]

Attachment: bug957052.patch

Sorry about the extra whitespace sneaking in, I thought I'd sorted that last time. Seems qrefresh doesn't always behave quite as I'd expect it to :)

Comment 11

[:Margaret Leibovic]

Comment on attachment 8358796 [details] [diff] [review]
bug957052.patch

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

Nice, this looks great! r+ with comments addressed.

::: mobile/android/chrome/content/aboutFeedback.js
@@ +30,5 @@
> +  document.getElementById("sad-link").addEventListener("click", function(evt) {
> +    switchSection("sad");
> +  }, false);
> +
> +  document.addEventListener("unload", uninit, false);

You should still add the unload listener to the window, not the document.

@@ +37,5 @@
> +  document.forms[0].addEventListener("submit", sendFeedback, false);
> +  document.getElementById("no-thanks").addEventListener("click", function(evt) {
> +    window.close();
> +  }, false);
> +  

Oops, a little more whitesapce :)

Comment 12

[Frederik Braun [:freddy]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #8)
> (In reply to Frederik Braun [:freddyb] from comment #5)
> > This is a tricky case of DOM Clobbering: Given an element with the ID |foo|,
> > the browser will automatically create |window.foo|
> 
> Not if there's already a window.foo, though, so that doesn't explain the
> failure.
> 
> The issue with the snippet in comment 4 is that event listeners are invoked
> with their "this" as the event target, and window.close doesn't like that.
> So you should keep the anonymous wrapper function (or bind window.close to
> window, but that's a bit uglier IMO).

Thanks for explaining this!

Comment 13

[Neil Bleasdale [:nbleasdale]]

Attachment: bug957052.patch moved event handlers from xhtml to js.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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