1067462 - Implement revised navigation UI

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[Giovanni Charles]

The navigation part of the FTE should have a list of options to go forward and a back button to go backwards, as per the spec.

https://mozilla.app.box.com/s/1dtk81iqslgnr8a69fmw

Comment 1

[Giovanni Charles]

Attachment: This patch changes the navigation from the 'gaia-buttons' implementation to a html list implementation

Comment 2

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8489639 [details] [review]
This patch changes the navigation from the 'gaia-buttons' implementation to a html list implementation

This is a pretty big patch, I've just scanned it for now and can offer some initial comments: 

* I'd like to see the patch split up a bit more if possible. The bug title doesn't really reflect the scope of the changes. Is it possible to break out the timezone changes and image removal into its own patch for example? 

* I'd like to see a short summary of the larger changes and their rationale. Either here or on the PR. Why the gaia-header to <header> change for example? What is the design that means the 'onlyForward' property on the steps is no longer necessary? That kind of thing. 

* Looks like you have some conflicts, can you rebase and resolve those? 

* You'll need to remove the console.logs eventually. 

* If there were other refactorings you thought might improve the codebase as you were developing this, I'd like to hear them. Feel free to file bugs or just mention them here. 

I'm looking forward to getting this in. We'll probably want to close out the last 2.1 FTU blockers before we land though (should be days not weeks) to avoid a lot of churn and uplift complication.

Comment 3

[Giovanni Charles]

*Splitting of the patch 

You can find the time/date patch here, Bug 1067636. I without the time date patch I would have to write ugly integration test code that I would only have to remove when the two branches merged again. But after looking at the diff I think it's a suitable tradeoff :/ 

Aside from that, I think the removal of the nav-bar forces the rest of the changes. Would it be preferable to have the nav-bar and the nav-list coexist for some page-specific patches?

*Large changes

gaia-header has unruly styling that other apps had solved by avoiding, so I thought I would follow suit. To make gaia-header work I use '!important' to override it's styling, there is a commit somewhere that does that, I can cherry pick it if you like.

The removal of onlyForward may be an early, dumb purge of anything related to the old nav-bar. I guess I also thought it looked ridiculous because it was set to true for everything but the first page. But it's not a necessary change.

* Keeping up to date - done, thanks. I'll try to keep up to date but this big patch touches a lot of code.

* Console logs gone from tests, thanks

* Refactoring 

The first thing that comes to mind is related to navigation.js. On each page change it sets up the new page by using manageStep, some kind of really large state transformer. Halfway through its work it changes window.location.hash which triggers a handler that does more, very similar, page setup! I had to resist serious urges to rewrite the whole thing.

My alternative would have involved some kind of transition function for each step, probably added to the `steps` mapping. This would replace the absurdly long logic in `manageStep`/`handleEvent` and make it clear what is executed on each transition.

I have a few other suggestions but I will post them on more relevant tickets and mention them in my PRs.

Thanks Sam :)

Comment 4

[Giovanni Charles]

These changes are now on the PR

Comment 5

[Sam Foster [:sfoster] (he/him)]

> * Refactoring 
> 
> The first thing that comes to mind is related to navigation.js. On each page
> change it sets up the new page by using manageStep, some kind of really
> large state transformer. Halfway through its work it changes
> window.location.hash which triggers a handler that does more, very similar,
> page setup! I had to resist serious urges to rewrite the whole thing.
> 
> My alternative would have involved some kind of transition function for each
> step, probably added to the `steps` mapping. This would replace the absurdly
> long logic in `manageStep`/`handleEvent` and make it clear what is executed
> on each transition.

Yes, I think everyone agrees the step management has grown into some spaghetti goto code, and it was high up on my list to fix also as soon as time allows. The number and sequence of steps changes on the fly based on different circumstances and user selections, but the business logic that drives this is difficult to follow and update at this point. I would be happy to look at prototypes if you are interested in taking this on, or just discuss (code) design options for a new implementation.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Giovanni, My apologies for stalling on this review. I'd like to get a bit further through 2.1 stabilization before landing this, as we are still seeing new bugs discovered affecting both 2.1 and master. In part I want to be able to focus on stabilization, but also the more those diverge, the harder it gets to develop and uplift patches for both branches. Its diminishing returns though and by next week we can probably push ahead - will that work for you?

Comment 7

[Giovanni Charles]

I can spend my last 2 days here tweaking the Gip tests and putting together some prototypes for step management.

For the following two weeks I will be away from my laptop/internet. It is my understanding that someone will take over, I will discuss this tomorrow with Aus. Regardless, I will catch up and continue when I get back on the 20th October.

Comment 8

[Fernando Campo (:fcampo)]

(In reply to Sam Foster [:sfoster] from comment #5)
> > * Refactoring 
> > 
> > The first thing that comes to mind is related to navigation.js. On each page
> > change it sets up the new page by using manageStep, some kind of really
> > large state transformer. Halfway through its work it changes
> > window.location.hash which triggers a handler that does more, very similar,
> > page setup! I had to resist serious urges to rewrite the whole thing.
> > 
> > My alternative would have involved some kind of transition function for each
> > step, probably added to the `steps` mapping. This would replace the absurdly
> > long logic in `manageStep`/`handleEvent` and make it clear what is executed
> > on each transition.
> 
> Yes, I think everyone agrees the step management has grown into some
> spaghetti goto code, and it was high up on my list to fix also as soon as
> time allows. The number and sequence of steps changes on the fly based on
> different circumstances and user selections, but the business logic that
> drives this is difficult to follow and update at this point. I would be
> happy to look at prototypes if you are interested in taking this on, or just
> discuss (code) design options for a new implementation.
Sorry for jumping in the discussion, just wanted to add that refactoring navigation has been a long waited step (bug 916826), never started because we had to focus on blockers and new features - the usual stuff, you know how it is.
Discussed with Borja and Guillaume months ago, we kind of designed the use of a Step object (bug 927807), similar to Tutorial, with a previousStep and nextStep pointer so we don't need to keep a step list (at least not as messy as the current one).
Hope it helps. And of course, if you want to discuss, I'm more than eager to do so (because I really really hate the navigation.js code :D)

Comment 9

[Giovanni Charles]

For the same reasons mentioned in Bug 1067677 I will have to give up this bug.

Comment 10

[Sam Foster [:sfoster] (he/him)]

I'm going to have a go at rebasing this and getting it landed. It makes significant markup and CSS changes that we want before we start on RTL in earnest for 2.2, and bug 1092965

Comment 11

[Sam Foster [:sfoster] (he/him)]

Initial rebasing done, I'm tracking this work in https://github.com/sfoster/gaia/tree/ftu-ui-redux-bug-1067462
Next up is getting tests to pass (and writing new tests where necessary). Plenty of scope for regressions in here, in particular with the recent l10n refactoring pass we did over the FTU. 

I'm also changing the title of this bug as the patch is pretty broad in scope and implements most of the proposed navigation UI changes in https://mozilla.app.box.com/s/1dtk81iqslgnr8a69fmw

Comment 12

[Sam Foster [:sfoster] (he/him)]

Attachment: UI, Navigation list changes

WIP getting Giovanni's patch rebased and ready for review

Comment 13

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8528069 [details] [review]
UI, Navigation list changes

This patch changes the way we build action lists and the next/back buttons in the FTU. Currently its just a <li class='nav-item'>..</li> basically. I'm thinking that's going to break a11y without some intervention. Can you offer guidance Marco?

Comment 14

[Marco Zehe (:MarcoZ)]

Comment on attachment 8528069 [details] [review]
UI, Navigation list changes

Thank you for the heads-up! This indeed breaks accessibility every instance a real button is converted into a plain text node. It loses the automatic "for free" ability to activate the control via screen reader, and it also loses the semantic of whether something is a radio button, a button or other element. I left come contextual comments on Github, too, but didn't go through the whole patch. NI'ing Yura since he did the heavy-lifting of the work to make the current FTU accessibile.

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

I agree with Marco in terms of the new markup becoming inaccessible. I would suggest using role="listbox" on the ul/ol container and role="option" on each li that represents a pass to the next step. The element with the role listbox MUST have an aria-label as well, something like "Next steps" would give the user enough context about what the list is about. After the above changes I would suggest turning the screen reader on and trying going through the FTU setup with it and checking that things make sense and are well understood.

Please mark me for a11y-review? on the pull request and I'll be able to check for any possible a11y regressions.

Comment 16

[Sam Foster [:sfoster] (he/him)]

I talked with jsavory about this as the more I played with it and started fixing these accessibility issues, the more problems I started to see. Turns out we've jumped the gun; this spec was not finalized, and getting it finished is not in scope for 2.2. So, regretfully, I'm going to call this a prototype, leave the attached PR and associated branch and hope that it doesnt bit-rot beyond all repair by the time this comes back up as a priority.

For future reference, here's the status and next steps for this patch: 

* UI changes which re-jigger the screens to make each option the 'forward' button are implemented (vs. the select option, then tap next paradigm currently used) 
* There are various markup changes. The l10n strings/property changes are nominally done, but not reviewed/f+'d. 
* Accessibility is currently broken. See comment #15 for suggested next steps to un-break it. 
* Unit tests, marionette-js and UI tests are all passing. They may need revisiting when a11y changes are implemented
* The patch contains some RTL accomodations, but these are probably not to spec.

Comment 17

[BMO Automation]

Firefox OS is not being worked on