Closed Bug 1068973 Opened 8 years ago Closed 8 years ago

Footer appears before thumbnails in markup

Categories

(Firefox OS Graveyard :: Gaia::Gallery, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S5 (6feb)
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

(Keywords: access, Whiteboard: [b2ga11y p=1])

Attachments

(1 file)

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.
Assignee: nobody → eitan
Before I ask for review, I just wanted to get some feedback and see if this is acceptable solution.
Attachment #8540280 - Flags: feedback?(pdahiya)
The pull request above also remedies bug 1068957.
Blocks: 1068957
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!
Attachment #8540280 - Flags: feedback?(pdahiya)
Attachment #8540280 - Flags: feedback?(dflanagan)
Attachment #8540280 - Flags: feedback+
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.
Attachment #8540280 - Flags: feedback?(dflanagan) → feedback-
(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.
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.
Attachment #8540280 - Flags: feedback?(dflanagan)
Attachment #8540280 - Flags: feedback-
Attachment #8540280 - Flags: feedback+
Blocks: 1068984
No longer blocks: 1068984
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.
Attachment #8540280 - Flags: feedback?(dflanagan) → feedback+
(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!
Attachment #8540280 - Flags: review?(pdahiya)
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+.
Attachment #8540280 - Flags: review?(pdahiya) → review+
https://github.com/mozilla-b2g/gaia/commit/2a00c429f244d05ee9943181a32bc01a836cd55e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Attachment #8540280 - Flags: approval-gaia-v2.2?
Attachment #8540280 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
You need to log in before you can comment on or make changes to this bug.