1145567 - When opening a page from Reader Mode panel, a grey bar is displayed before the reader mode toolbar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 39.0a1 (2015-03-20)
Steps to reproduce:

1. Open an article from news.google.com
2. Enter reader mode and add to reading list
3. Close the tab and open it from the Reader Mode panel

Expected results:
- The reader mode toolbar slides off the screen.

Actual results:
- A grey bar is displayed before the reader mode toolbar slides off the screen.

Comment 1

[Teodora Vermesan (:TeoVermesan)]

Please look at the following video: https://www.youtube.com/watch?v=nOYjLrhtHxc&feature=youtu.be

Comment 2

[Aaron Train [:aaronmt]]

Please remember to nominate regressions for tracking.

Comment 3

[Teodora Vermesan (:TeoVermesan)]

good build: 13-03 (reader mode jumps when opened from about:home panel: see Bug 975533)
bad build: 14-03 (a grey bar is displayed before the toolbar, now the toolbar slides off the screen). 

pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=42afc7ef5ccb&tochange=38154607d807

It is definitely Bug 998031 - Reader Mode toolbar should scroll in and out instead of fading

Comment 4

[Teodora Vermesan (:TeoVermesan)]

I've also commented in Bug 975533: https://bugzilla.mozilla.org/show_bug.cgi?id=975533#c8

Comment 5

[:Margaret Leibovic]

This bar is such a pain. I suspect this is just the new manifestation of bug 975533.

I tried working around that in bug 998031, but apparently I didn't do a good enough job.

I feel like this is a layout/rendering issue, but we'll probably just need to find another way to work around it.

Comment 6

[:Margaret Leibovic]

(This will be affected on 38 when I uplift 998031)

Vivek, maybe this would be another reader view you would be interested in looking at?

Yes, this is technically a regression from bug 998031, but I feel like in reality it's just a different manifestation of bug 975533.

Comment 7

[Vivek Balakrishnan[:vivek]]

Thanks, I'll take it

Comment 8

[Vivek Balakrishnan[:vivek]]

Attachment: 1145567.patch

The issue seems like the css transistions are triggered even before DOM contents are loaded. so. the patch updates the toolbar display property as late as possible.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8587612 [details] [diff] [review]
1145567.patch

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

::: toolkit/components/reader/AboutReader.jsm
@@ +270,2 @@
>              // Hacks! Delay showing the toolbar to avoid position: fixed; jankiness. See bug 975533.
>              this._win.setTimeout(() => this._setToolbarVisibility(true), 500);

So, I already added this setTimeout to try to avoid the exact issue in this bug. Can you try removing this setTimeout to see if the bug is still fixed? If so, you did a better job than me :)

However, I'm still a bit wary of this fix, since it depends on the fact that it takes a bit of time for the message to come back from Java. Maybe we do still need the setTimeout, in which case we should set the display inside the timeout callback.

In any case, I think we should update this patch to either get rid of the timeout altogether, or move setting the display inside of the timeout callback.

I also think you should add a comment explaining that we delay making the bar visible to avoid this layout bug.

Comment 10

[Vivek Balakrishnan[:vivek]]

Attachment: 1145567.patch

Patch updated with comments. As discussed over irc, setTimeout is now done to have a nicer slide up animation.

Comment 11

[:Margaret Leibovic]

Comment on attachment 8587695 [details] [diff] [review]
1145567.patch

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

I tested this locally, and I'm still seeing the issue of the bar appear early with a janky animation, so I still think we need a delay before we make the bar visible. However, I think you're on the right path here by setting the bar to just not be visible, since it seems like the root problem here is that we're not properly rendering this bar off screen initially.

I tried putting the call to set the display inside the setTimeout, above the _setToolbarVisibility call, and that seemed to fix the incorrect positioning issue, but the bar appears without the transition the first time it is shown, which is not what we want. We could add *another* setTimeout, but that feels pretty terrible. I'm not quite sure what to do here. Maybe we should just compromise and not have the transition the first time the bar is shown.

FYI, I'm testing on a Nexus 4 running Android 5, and I'm worried this might also be a worse issue on lower-end phones :/

Comment 12

[Vivek Balakrishnan[:vivek]]

Attachment: 1145567.patch

Added nested timeouts to delay displaying the toolbar and then have a nice bottom-up animation.

Comment 13

[:Margaret Leibovic]

Comment on attachment 8591023 [details] [diff] [review]
1145567.patch

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

This works great testing locally. This also appears to fix some issues I've seen previously where tapping on the toolbar initially won't work.

::: toolkit/components/reader/AboutReader.jsm
@@ +269,5 @@
>            this._updateToggleButton();
>  
>            // Display the toolbar when all its initial component states are known
>            if (isInitialStateChange) {
> +            // Toolbar display is updated here to avoid the grey bar. see bug 1145567.

Since the "grey bar" we're seeing is indeed the toolbar, but mispositioned, I would update this to say something like like "Toolbar display is updated here to avoid it appearing in the middle of the screen on page load. See bug 1145567."

Comment 14

[Vivek Balakrishnan[:vivek]]

Attachment: 1145567.patch

Comment updated as per the review suggestion.

Comment 15

[:Margaret Leibovic]

Comment on attachment 8591083 [details] [diff] [review]
1145567.patch

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

Looks good to me, I can land this for you.

Comment 16

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/c1b57c493993

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c1b57c493993

Comment 18

[:Margaret Leibovic]

Comment on attachment 8591083 [details] [diff] [review]
1145567.patch

Approval Request Comment
[Feature/regressing bug #]: bug 998031

[User impact if declined]: Reader view toolbar will appear in the wrong place on first load.

[Describe test coverage new/current, TreeHerder]: Tested locally and landed on m-c, no automated testing.

[Risks and why]: We're just adding hacks on hacks here, but this should be low risk, since it's a small change to just add another timeout before showing the reader view toolbar.

[String/UUID change made/needed]: none

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8591083 [details] [diff] [review]
1145567.patch

Approving for uplift to aurora since this sounds low risk, we need it for Reading List, and it looks ok on treeherder.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8ecc0e6da50

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8591083 [details] [diff] [review]
1145567.patch

Should be in 38 beta 5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/df47a99c442f

Comment 23

[Teodora Vermesan (:TeoVermesan)]

No grey bar is displayed before reader mode toolbar slides off the screen, so:
Verified as fixed using:
Devices: Nexus 7 (Android 5.0) and Samsung S5 (Android 4,4,4)
Builds: Firefox for Android 40.0a1 (2015-04-15) and Firefox for Android 39.0a2 (2015-02-15)