Eitan Isaacson [:eeejay] Assignee
	Description • 10 years ago

Footer should be the last element in the markup, since they are visually below the thumbnails. This makes things less confusing for screen reader users.

	Eitan Isaacson [:eeejay] Assignee
	Comment 1 • 10 years ago

Before I ask for review, I just wanted to get some feedback and see if this is acceptable solution.

	Eitan Isaacson [:eeejay] Assignee
	Comment 2 • 10 years ago

The pull request above also remedies bug 1068957.

	Punam Dahiya [:pdahiya]
	Comment 3 • 9 years ago

Comment on attachment 8540280 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26955

Hi Eitan 
Sorry for delay in feedback. It looks good and has my feedback+, few nits that I have noted in github. It will be good to get it tested in tablet as it's touching tablet css. 
I am including djf in the loop to get his feedback on grouping in case I have missed something. Thanks!

	David Flanagan [:djf]
	Comment 4 • 9 years ago

Comment on attachment 8540280 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26955

Eitan,

It took me a while to figure out what you were doing here because the layout code has changed a lot since I last worked on it...

I don't like what you've done here because there is no longer one <section> element for each of the possible views. What I propose instead is that you use a structure like this:

<!-- this section is used for list view, select view and pick view -->
<!-- all the views that display the list of thumbnails -->
<section role="region" id="thumbnail-views" class="skin-dark">

 <section role="region" id="thumbnail-headers" class="skin-dark">
  <!-- list, select, and pick headers -->
 </section>

 <ul id="thumbnails"></ul> <!-- this element used to be below -->

 <section role="region" id="thumbnail-footers">
   <!-- footers for list and select views -->
 </section>
</section>

This is pretty much what you've got now, but it includes the pick view as well, and it puts the headers, thumbnails, and footers in a common container.

Does that make sense and seem do-able?

It was the need to share the thumbnails element among these three views that caused this weird layout. Thanks for fixing it.

	Eitan Isaacson [:eeejay] Assignee
	Comment 5 • 9 years ago

(In reply to David Flanagan [:djf] from comment #4)
> Comment on attachment 8540280 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26955
> 
> Eitan,
> 
> It took me a while to figure out what you were doing here because the layout
> code has changed a lot since I last worked on it...
> 
> I don't like what you've done here because there is no longer one <section>
> element for each of the possible views. What I propose instead is that you
> use a structure like this:
> 
> <!-- this section is used for list view, select view and pick view -->
> <!-- all the views that display the list of thumbnails -->
> <section role="region" id="thumbnail-views" class="skin-dark">
> 
>  <section role="region" id="thumbnail-headers" class="skin-dark">
>   <!-- list, select, and pick headers -->
>  </section>
> 
>  <ul id="thumbnails"></ul> <!-- this element used to be below -->
> 
>  <section role="region" id="thumbnail-footers">
>    <!-- footers for list and select views -->
>  </section>
> </section>
> 
> This is pretty much what you've got now, but it includes the pick view as
> well, and it puts the headers, thumbnails, and footers in a common container.
> 
> Does that make sense and seem do-able?
> 
> It was the need to share the thumbnails element among these three views that
> caused this weird layout. Thanks for fixing it.

This makes sense. I'll try something new, and propose it soon.

	Eitan Isaacson [:eeejay] Assignee
	Comment 6 • 9 years ago

Comment on attachment 8540280 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26955

I rearranged the markup to be as you suggest. With the only difference being that I didn't put the headers and footers in sections of their own.

	David Flanagan [:djf]
	Comment 7 • 9 years ago

Comment on attachment 8540280 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26955

Thanks for re-organizing this. It makes much more sense to me now. My only feedback on this version is that a comment on the "thumbnail-views" section would be nice. See github.

I have not tested this and will leave final review to Punam.

	Eitan Isaacson [:eeejay] Assignee
	Comment 8 • 9 years ago

(In reply to David Flanagan [:djf] from comment #7)
> Thanks for re-organizing this. It makes much more sense to me now. My only
> feedback on this version is that a comment on the "thumbnail-views" section
> would be nice. See github.
> 

Added. Thanks!

	Punam Dahiya [:pdahiya]
	Comment 9 • 9 years ago

Comment on attachment 8540280 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26955

Thanks Eitan for working on this patch. Tested and its r+.

	Eitan Isaacson [:eeejay] Assignee
	Comment 10 • 9 years ago

https://github.com/mozilla-b2g/gaia/commit/2a00c429f244d05ee9943181a32bc01a836cd55e

	Eitan Isaacson [:eeejay] Assignee
	Comment 12 • 9 years ago

Comment on attachment 8540280 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26955

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: Screen reader users will perceive an illogical layout where the thumbnails appear after the footer.
[Testing completed]: Yes. Tested in tablet layout as well.
[Risk to taking this patch] (and alternatives if risky): Low-medium since this is not a small markup change.
[String changes made]: None.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 9 years ago

v2.2: https://github.com/mozilla-b2g/gaia/commit/5723defdcc1d398df887f1657968d6b3315ff02f