Amphtml: amp-story: Progress bar bugs

[hongcatlover]

What's the issue?

• [ ] The first page should show 1 progress bar segment filled instead of 0.
• [ ] The last page (before the bookend appears) should show all progress bar segments filled.
• [ ] The progress bar sometimes appears to rewind twice (two segments instead of one) for stories with auto-advance enabled.

What browsers are affected?

Google Chrome
iOS safari

cc: @alanorozco @newmuis @choumx @prateekbh

EDIT(@newmuis): Merging in #12107

Progress bar is not updating or sometimes has missing pieces in the middle on slow connection and when navigating quickly between pages.

Here's a screenshot of the progress bar:

[choumx]

@sanjsanj Would you mind taking a look at this? The first two bugs are hopefully straightforward enough. Thank you!

[sanjsanj]

@choumx Absolutely! Won't have time to have a look until tomorrow night but I'll keep you posted 👍

[choumx]

Woo! 😄

[alanorozco]

Adding to this, UXD needs to be updated for auto-advance when the initial state for an active page is full.

[sanjsanj]

@choumx Haven't had time yet but I will definitely look at it if you can bear with me a few more days. Christmas stuff and too much work-work!

[choumx]

No worries. Merry Xmas!

[sanjsanj]

@choumx I finally had a couple of hours to have a look at this issue, and above is my WIP PR #12613 , not sure if it's the right approach or not but wanted to get some initial feedback when you have a chance please, TY!

I note that progress bar segments do fill when auto-advance-after attribute is present in the html thanks to TimeBasedAdvancement and take this issue to change the behaviour of pages that only have manual advancement.

https://github.com/ampproject/amphtml/blob/502405f5988c4a8b1d653d162f3b071c22d18631/extensions/amp-story/0.1/page-advancement.js#L140-L144

My approach was to call onProgressUpdate() in ManualAdvancement.start() and it appears to work... until I hit replay on the bookend and then the final segment stays filled until the 2nd segment gets called again. This is the only thing that's not working right now and I'm not sure why (yet), it'll be another 2 or 3 days before I have time to code again :)

EDIT: Found that the last page's start() method gets called when the bookend's replay button is clicked and that's what's causing the above issue.

I do have some questions if I may:

1 - Should the cover page have a progress bar segment and is that what exists now? e.g. If there's a cover page and 2 other pages then we expect to see 3 progress bar segments?
2 - I don't see a progress bar segment for the bookend, am I missing something?
3 - @alanorozco 's PR #12604 fixes the issue he identified and might have an impact on this issue after merged in, I'll have to check when that happens.

[choumx]

@sanjsanj Happy new year! Just got back into the office. Will take a closer look at your questions tomorrow.

[sanjsanj]

@choumx Happy New Year! No rush at all, speak to you when you have time

[choumx]

1 - Should the cover page have a progress bar segment and is that what exists now? e.g. If there's a cover page and 2 other pages then we expect to see 3 progress bar segments?

Yes and yes. Upon landing on the cover (first) page, the first segment should already be filled. Currently, the progress bar is completely empty on the cover page.

2 - I don't see a progress bar segment for the bookend, am I missing something?

On the last page of a story (before the bookend appears), the progress bar should still have one segment missing. Navigating forward again will display the bookend, but the progress bar is still short one segment.

3 - @alanorozco 's PR #12604 fixes the issue he identified and might have an impact on this issue after merged in, I'll have to check when that happens.

I think his PR addresses the third checkbox in the issue (double rewind), so it shouldn't impact your work.

Hope that helps. Thanks again!

[hongcatlover]

Re:2 - I don't see a progress bar segment for the bookend, am I missing
something?

On the last page of a story (before the bookend appears), the progress bar
should still have one segment missing. Navigating forward again will
display the bookend, but the progress bar is still short one segment.

Are we still counting bookend as one segment in progress bar? I agree that
having an empty bar may give a higher chance for the bookend to be
triggered, but it is weird to count one overlay as same as a page? In our
last round user testing, everyone triggered the bookend by keep tapping to
the next.

If we do want to show any visual cue for the bookend only, I think we
should treat the UI differently in the progress bar, or automatically
animate in the bookend after X time but keep progress bar all filled on the
last page. I emailed Chris about this, would love to have a discussion
first.

On Thu, Jan 4, 2018 at 12:03 PM William Chou [e-mail]
wrote:

1 - Should the cover page have a progress bar segment and is that what
exists now? e.g. If there's a cover page and 2 other pages then we expect
to see 3 progress bar segments?

Yes and yes. Upon landing on the cover (first) page, the first segment
should already be filled. Currently, the progress bar is completely empty
on the cover page.

2 - I don't see a progress bar segment for the bookend, am I missing
something?

On the last page of a story (before the bookend appears), the progress bar
should still have one segment missing. Navigating forward again will
display the bookend, but the progress bar is still short one segment.

3 - @alanorozco https://github.com/alanorozco 's PR #12604
https://github.com/ampproject/amphtml/pull/12604 fixes the issue he
identified and might have an impact on this issue after merged in, I'll
have to check when that happens.

I think his PR addresses the third checkbox in the issue (double rewind),
so it shouldn't impact your work.

Hope that helps. Thanks again!

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/ampproject/amphtml/issues/12412#issuecomment-355384339,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AgJEkkNUU3MTXLuTcpmK74uGtZm5Xfn0ks5tHS6IgaJpZM4Q91SM
.

[choumx]

I think we're actually in agreement. I was referring to the last, unfilled segment as belonging to the bookend. Reworded the bug for clarity.

[hongcatlover]

Oh! Sorry! Guess I misunderstood your comments. Progress bar only counts by page level; then we will be all good.

[sanjsanj]

Thanks both for the feedback! Finally going to have time again this week to look at this issue. Be in touch soon, cheers!

[newmuis]

@hongwei1990 I don't think I get the third line item here. Shouldn't the progress bar rewind twice in some cases?

For pages that are auto-advancing, the bar starts empty.
For pages that are manually advanced, the bar starts full.

So, given a story with this construction:

• page1: manual advance
• page2: auto advance
• page3: manual advance

If I am on page3, all three bars should be filled. If I then go to the previous page (page2), the bar for page3 should become empty (because I am before that page) and the bar for page2 should also become empty, because it is beginning the auto-advance.

Is this not correct?

[sanjsanj]

Just so we're all on the same page, the bookend does not have a progress bar segment and this is how we want the progress bar to behave:

All manual-advancement:

Auto-advance cover page, auto-advance page 1, manual-advance page 2:

Rewinding to an auto-advance page:

:-)

[choumx]

Can't see those images. 🙁

[sanjsanj]

Yeah sorry, let me fix that now :)

edit: fixed

[newmuis]

These all look correct to me; @hongwei1990, can you double-check?

[hongcatlover]

SHARP! Looks awesome! Thanks so much for your help on this!

[sanjsanj]

Awesome! And no worries, it's my pleasure to learn from your codebase. It might still take me a couple of weeks, I haven't looked into the segment-skipping issue and I have a bug... and I only get the rare hour or so a night now to code :)

Also the MediaBasedAdvancement wasn't working for me, is it working in master? I tweaked the selector and it now works though, so just want to confirm this is the desired behaviour for media progress-bar advancement:

The bug I'm experiencing is the replay button on the bookend triggering the progress-bar segment for the last page before triggering it for the first, still looking into it:

The above only happens via replay on bookend, works fine when there's no bookend. The solution is evading me so far but I'll carry on tomorrow! G'night!

[newmuis]

Hi @sanjsanj! The screencap you posted does indeed look like the correct behavior for media advancement. Thanks for taking a look! 😄

Let me know if you need help digging into the replay issue.

[sanjsanj]

Cool, thanks for having a look @newmuis! I'm making some progress tonight so leave it with me and I'll come back to you guys for help soon as I need it.