811745 - [Wallpaper] The app and the settings screen need polish

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect, P2)

Description

[Patryk Adamczyk [:patryk] UX]

The app nor the settings portion of wallpaper selection were ever designed. This bug address this.

Comment 1

[Eric Pang [:epang] UX]

Attachment: Mock Ups and Specs of the Wallpaper Settings Page

Comment 2

[Eric Pang [:epang] UX]

Attachment: Edit Button Icons

For use on Wallpaper Settings page.  Two states: 1) Normal 2) Pressed

Comment 3

[Eric Pang [:epang] UX]

Attachment: Mock up and Specs of Wallpaper Selection Screen

After clicking the edit button on the wallpaper settings screen you see this selection screen.

Comment 4

[Sam Joch [:samjoch]]

I need more updates about the "Screen Timeout" button https://github.com/guiora/gaia/tree/bug-811745

Comment 5

[Eric Pang [:epang] UX]

Attachment: mock ups of screen time out button

Sam, I've added corrected mock ups for screen timeout. Let me know if you have questions. thx

Comment 6

[Gordon Brander :gordonb]

(In reply to Eric Pang [:epang] from comment #5)
> Created attachment 684436 [details]
> mock ups of screen time out button

It looks like the "normal state" and "pressed" are reversed here. Is this intentional? Also, I believe the border-radius on the dropdown may be smaller here than in other comps.

Comment 7

[Sam Joch [:samjoch]]

Attachment: polishing wallpaper

Comment 8

[David Flanagan [:djf]]

Sam,

I can't apply the patch to the tip in git, which will make it hard to review. Is this a diff against the UX branch, maybe?

If you can rebase it so I can apply it to the master branch that would be great. Otherwise, I'll review the code without testing it, but will ask you to get a second review from someone working on the UX branch who can actually test it.

Comment 9

[David Flanagan [:djf]]

Comment on attachment 685617 [details] [diff] [review]
polishing wallpaper

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

I'm really glad that someone is working on cleaning this up. When I wrote it, I just did the simplest thing that worked.

I can't give a r+ without code that applies cleanly, though, and I had too many questions about the code.

Next steps:

1) rebase against the master branch so I can try it out (or tell me what I can apply it against. I don't know anything about the UX branch, or how to work against it)

2) Explain why the screen timeout thing needs a select element and a button.  I just don't get what that is doing.

3) Try to do the wallpaper picking stuff without all that javascript code for sizing and positioning the images.

4) Start using gjslint on your js code, and watch out for trailing whitespace.  I don't really know why we're so picky about those things, but we are.

::: apps/settings/index.html
@@ +896,4 @@
>              <span class="range-icons brightness"></span>
>            </label>
>          </li>
> +        <li id="screen-timeout">

Is it itentional that there is no header here?

@@ +896,5 @@
>              <span class="range-icons brightness"></span>
>            </label>
>          </li>
> +        <li id="screen-timeout">
> +          <button class="icon icon-dialog"></button>

I don't understand why there is a button here. Why isn't a normal select element sufficient?  

Are you just trying to get different visual design than what the select gives?  

When I look at https://bug811745.bugzilla.mozilla.org/attachment.cgi?id=684436 I just see a label and a select element.  Or is that a button that looks like a select element?

In any case I don't understand why there is a button and a select. If this is some normal idiom that the settings app uses, then its fine, I suppose. But if you're doing something tricky here, I'm skeptical.

I have similar questions about the corresponding CSS.  Why is the select absolutely positioned and the button just part of normal flow.

Basically I don't know what is going on, and since the patch won't apply I can't try it to figure it out.

::: apps/settings/js/screen_timeout.js
@@ +1,1 @@
> +'use script';

That should be use strict, not use script!

This file is much more complicated than it needs to be.  Nothing else is ever going to call any of these ScreenTimeout methods, so this could all be inside an anonymous self-invoking function.  E.g.:

(function() {
  var el = document.getElementById(...)
  ...
  function update() {...}
  select.addEventListener('change', update);
  update();
}())

Also, the extra spaces on lines 10, 16, and 20 won't get by gjslint and should be removed.

::: apps/settings/js/wallpaper.js
@@ +33,5 @@
>    bindEvent: function wallpaper_bindEvent() {
>      var self = this;
>      var settings = navigator.mozSettings;
> +    var onWallpaperClick = function wallpaper_onWallpaperClick() {
> +      

nit: get rid of trailing whitespace

@@ +76,5 @@
> +    };
> +    this.preview.addEventListener('touchstart', onTouchStart)
> +    this.preview.addEventListener('touchend', onTouchEnd);
> +    this.button.addEventListener('touchstart', onTouchStart)
> +    this.button.addEventListener('touchend', onTouchEnd);

If you have to go to this much work with the touch events to get the effect you want, I suspect that you're doing something wrong, or the effect is overdesigned...

I say just use button:active in the stylesheet intead of setting the hover class. Then when the user's finger goes down on the button, they'll get the feedback.  If you really want to highlight the button when the user's finger is on the wallpaper (do you?) you can probably do that with CSS3, too, with wallpaper:active+button or something like that.

I really don't think you need these event handlers here

::: apps/settings/style/settings.css
@@ +177,5 @@
> +  width: 45px;
> +  right: 1rem;
> +  margin: 0;
> +}
> +#wallpaper-button.hover {

try #wallpaper-button:active here, instead, and get rid of the event handlers that set and clear the hover class.

@@ +185,4 @@
>  #wallpaper-preview {
> +  margin-top: -10rem;
> +  position: absolute;
> +  width: 100%;

Are you just relying on the top and the bottom of the 
image being chopped off? I couldn't understand that at first. A comment explaining that would be helpful

@@ +192,5 @@
> +  position: relative;
> +}
> +
> +#screen-timeout select {
> +  position: absolute;  

nit: trailing white space.

Why is absolute positioning needed here?

::: apps/wallpaper/js/pick.js
@@ +38,5 @@
> +      var el = document.getElementById('wallpapers');
> +      if(el) { el.style.height = 'auto'; };
> +    }
> +
> +};

I don't think that any of this new code is actually necessary.

All of the wallpaper images are already at a 2:3 aspect ratio, and even if it wasn't you could use background-size:cover to give you just the middle part if you need it.

You also don't need to dynamically set the position of each icon. If you set the size of the images correctly, you'll get three to a row without having to put each one in place.

Finally, even if we did need this code, we don't use the comma-first style anywhere else in Gaia, and I'd probably have to ask you to change that style before landing. (I'm generally very forgiving about style, but this is one where you kind of have do it all the way or not do it...)

@@ +44,5 @@
>  window.onload = function() {
> +
> +  Wallpaper.init();
> +
> +  navigator.mozSetMessageHandler && navigator.mozSetMessageHandler('activity', function handler(activityRequest) {

gjslint will complain that this line is too long.

I don't like the style of using && like this.  If you really want to check for the existance of mozSetMessageHandler, I'd do it with

if (!navigator.mozSetMessageHandler)
    return;

@@ +67,4 @@
>      if (!(e.target instanceof HTMLImageElement))
>        return;
>  
> +    if(!pickActivity) { return; }

Before we land any gaia code, we run gjslint over it. It is a style checker from google, and you should get it and run it before you submit pull requests.

In this case, it would complain about no space after if, and possibly also about putting everything on one line.

I find it annoying, but it is something we're all subjecting ourselves to.

::: apps/wallpaper/pick.html
@@ +22,4 @@
>        </header>
>      </section>
>      <div id="wallpapers">
> +      <span class="wallpaper"><img src="resources/320x480/FXOS_Blue_Mountains.png"/></span>

Do you really need the separate span, or can you just put the class on the img directly?

Or instead, can you set the image as a background-image on the span?

Comment 10

[David Flanagan [:djf]]

Sam,

My apologies. Your patch does apply cleanly. I tried to create a new branch to apply it on and ended up checking out some old branch of mine. And it didn't apply to my out of date branch.

Comment 11

[David Flanagan [:djf]]

Okay, so now I can see the patch in action. Here are my thoughts:

1) The patch doesn't include the images for the wallpaper select button, so that doesn't appear for me.  If you have a github pull request, you can just use a link to the PR as the bugzilla attachment, and that way the images will be included, too.

2) I still don't understand what is going on with the button and the select element. It looks like you're making the select element look different than all the other standard select elements in the settings app. I don't get it.

Comment 12

[Sam Joch [:samjoch]]

David, 

About the button, i use it to match the building block style where i didn't find any select sample.
But you right it doesn't make sense to use button and a select. I'll use a label instead.

Here is the PR link: https://github.com/mozilla-b2g/gaia/pull/6653

Comment 13

[Sam Joch [:samjoch]]

Attachment: David, i cleaned the code throught your recommendations.

Comment 14

[David Flanagan [:djf]]

Sam,

I'm working on a major bug for the C2 deadline this weekend, so I may not be able to do this review until early next week.  Just FYI.

Comment 15

[David Flanagan [:djf]]

Comment on attachment 689252 [details] [diff] [review]
David, i cleaned the code throught your recommendations.

Sam, 

This is getting closer, but still not ready to land.

Most of my comments are small optional suggestions.

One major issue though is in pick.js: since you switched from <img> elements to <div> elements with background-image, you have to wait for an onload() event before before copying the wallpaper to the canvas to create the blob.

See my comments on github.

Comment 16

[Sam Joch [:samjoch]]

David, 

I included the onload() and made few updates.

All my updates have been pushed.
https://github.com/mozilla-b2g/gaia/pull/6653

Comment 17

[David Flanagan [:djf]]

Comment on attachment 689252 [details] [diff] [review]
David, i cleaned the code throught your recommendations.

Sorry this review has taken a while. I didn't know you had updated github until you set the review flag here on bugzilla, so it wasn't until this morning that I realized you wanted a review again.

I'm giving r- again because I think this patch uses too much custom JavaScript and alters the default behavior of HTML elements in order to acheive minor visual effects that aren't being applied elsewhere in Gaia.

See my comments on github, but basically: please just make the screen timeout select box an ordinary box without custom JavaScript and CSS. If we need to change the way selects look or behave, we should do that in gecko so it applies to all select boxes.  And also, please remove the custom event handlers for wallpaper selection. The CSS :active pseudo-class is exactly the right thing here and altering it with JavaScript will just confuse users.

Comment 18

[Patryk Adamczyk [:patryk] UX]

Hey David, can you clarify what you're rejecting here?
1. The correct common control for drop downs... https://bug811745.bugzilla.mozilla.org/attachment.cgi?id=684436 This was never implemented correctly, how should Sam fix this? Should this be name a common control.
2. Wallpaper selection UI. If so, what exactly is blocking the merge of this? Too much custom javascript? If we can't get this into 1.0, can we merge this into 1.1? How?

Comment 19

[Patryk Adamczyk [:patryk] UX]

^ Should this be name a common control. = Should this be made a common control.

Comment 20

[Patryk Adamczyk [:patryk] UX]

I am escalating this as with the addition of the common control this is becoming a bigger issue.

Comment 21

[David Flanagan [:djf]]

(In reply to Patryk Adamczyk [:patryk] UX from comment #18)
> Hey David, can you clarify what you're rejecting here?
> 1. The correct common control for drop downs...
> https://bug811745.bugzilla.mozilla.org/attachment.cgi?id=684436

I don't know anything about how select elements are supposed to look. But this patch makes the one screen timeout select element look different than every other select element in the settings app.  And doing that requires a file of custom javascript code, an extra span element, and something like 30 lines of CSS.

I'm assuming that: 

1) You don't really want to have one select element that looks different than every other select element in Gaia

2) We can't ask developers to tweak all of their select elements by adding custom HTML, JS and CSS for every single one.

There must be a way to change the way that gecko renders select elements. I don't know what it is, but it should be done so that it works universally and does not require the contortions that this patch goes through.

> This was
> never implemented correctly, how should Sam fix this? 

I think he should just revert the patch to use a plain old select element like it use to be.  Then file a separate bug for making all select elements look the way you want them too.

> Should this be made a
> common control.

I don't actually know what a common control is. The building blocks stuff we have is pure CSS. And if there was a set of CSS styles we could use on select elements, that would be great.

But what Sam has done here requires custom JavaScript and HTML for each select element as well, and I don't think we have any mechanism in gaia for "common controls" that require that much customization.


> 2. Wallpaper selection UI. If so, what exactly is blocking the merge of
> this? Too much custom javascript? If we can't get this into 1.0, can we
> merge this into 1.1? How?

Sam is using event handlers to hightlight the wallpaper select button whenever your finger is over the wallpaper rather than just using the CSS :active psuedo-class which is what we normally use for this sort of thing.  He does this because he wants the highlight to appear not just when you touch the wallpaper, but also when you drag your finger around the wallpaper.

The problem is, that if you touch the wallpaper then drag your finger around it, and then release, you will not have triggered the "click" event that is required to actually select new wallpaper.  So the persistent highlight that the patch achieves actually misleads the user into thinking that they can select wallpaper when they cannot.

For simplicity of implementation and for consistency with all other buttons out there, I think it would be better to just highlight the button with CSS :active and get rid of the custom JavaScript tweak.

Alternatively, the custom JavaScript tweak should be modified so that it actually selects wallpaper when you lift your finger even after a drag. But I fear that that will be trickier than it sounds.

Comment 22

[Sam Joch [:samjoch]]

Attachment: Select tag (no style way)

David,

This is what i get when i reset the style on the select tag (see attachment).
[Same html/css in Settings -> Language]

Should i leave it this way, and open a new bug about this issue ?

Comment 23

[David Flanagan [:djf]]

Comment on attachment 694405 [details]
Select tag (no style way)

No, don't allow the elements to overlap like this.

When I run the settings app without any of your patches, I see a line break between Screen timeout and the select elements.  I think you should do it that way.  

Remember also that even if it fit on one line in English, a line break might be necessary for other languages anyway.

Comment 24

[Sam Joch [:samjoch]]

Comment on attachment 689252 [details] [diff] [review]
David, i cleaned the code throught your recommendations.

I reset the select tag just like it was. 
And remove the touch events.

Comment 25

[David Flanagan [:djf]]

Comment on attachment 689252 [details] [diff] [review]
David, i cleaned the code throught your recommendations.

Sam,

Thank you for your patience going through this review process. You were getting conflicting messages from the design team and the programming team. In general right now, we're concerned about performance and startup time, which is why I really don't want us adding new javascript files or running any more code than we need to.

I'm giving an r+ now, but have left some comments on github that I'd like you to address before landing:

1) There's still a question pending on settings.css: you have 14rem where I think you should have 15rem (7.5 + 7.5).  I could be wrong about that.

2) There is style lint in pick.js that you should fix

3) In pick.js, I've actually requested a couple of actual code changes. Let's use the actual size of the wallpaper image rather than the screen size or requested size so that we don't distort these highly tuned PNGs.  And you should probably use a regexp for parsing backgroundImage. (You may have been doing that to begin with, and if I told you to take it out I apologize)

Comment 26

[Sam Joch [:samjoch]]

Thanks David, i understand your point of view.

For the css i put 15rem. The gjslint give me no error (with the --nojsdoc flag) after a fixjsstyle.
Now, i use the width/height's wallpaper to resize the canvas and i use the Regexp to match the wallpaper's url.

Comment 27

[Sam Joch [:samjoch]]

Attachment: PR Github

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky):

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 696285 [details]
PR Github

https://github.com/mozilla-b2g/gaia/commit/e3b58d03c3b5954e4eea24620ede94d363c91a7c