1155083 - A portion of the reader view toolbar is still displayed when scrolling down on landscape

Status: VERIFIED FIXED

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

Description

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-04-16-10-51-19.png

Tested with:
Device: Nexus 7 (Android 5.0)
Steps to reproduce:
1. Go to http://goo.gl/GdDxaP on landscape
2. Enter reader view by tapping the reader icon from URL Bar
3. Scroll down the article

Expected results:
- Reader view toolbar disappears

Actual results:
- A portion of the toolbar is still displayed

Comment 1

[Catalin Suciu [:csuciu]]

Same behavior on Sony Xperia Z2 10" (4.4.4)

Comment 2

[Teodora Vermesan (:TeoVermesan)]

2015-03-13 not affected
2015-03-14 affected
pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=42afc7ef5ccb&tochange=38154607d807

Regression from Bug 998031 - Reader Mode toolbar should scroll in and out instead of fading

Comment 3

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1155083/margaret

/r/7173 - Bug 1155083 - Properly hide reader view tablet on landscape tablets. r=bnicholson

Pull down this commit:

hg pull -r fb5340e8c10360d3f246955354935f8fdd49b007 https://reviewboard-hg.mozilla.org/gecko/

Comment 4

[:Margaret Leibovic]

Comment on attachment 8593641 [details]
MozReview Request: bz://1155083/margaret

/r/7173 - Bug 1155083 - Properly hide reader view tablet on landscape tablets. r=bnicholson
/r/7175 - Bug 1154028 - Used scoped styles for reader view browser UI

Pull down these commits:

hg pull -r 1bb8d2facc875e061db2d58fcf6281842030a4fa https://reviewboard-hg.mozilla.org/gecko/

Comment 5

[Brian Nicholson (:bnicholson)]

https://reviewboard.mozilla.org/r/7173/#review5987

Change looks OK to me, though I'm a bit confused by Review Board. It includes bug 1154028, though clicking it says it's discarded (but why does it still appear)?

Anyway, r=me assuming this lands only the top commit.

Comment 6

[Brian Nicholson (:bnicholson)]

https://reviewboard.mozilla.org/r/7173/#review5989

Ship It!

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 8593641 [details]
MozReview Request: bz://1155083/margaret

https://reviewboard.mozilla.org/r/7171/#review5991

Let's try this one more time...

Comment 8

[Pulsebot]

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

Comment 9

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #5)
> https://reviewboard.mozilla.org/r/7173/#review5987
> 
> Change looks OK to me, though I'm a bit confused by Review Board. It
> includes bug 1154028, though clicking it says it's discarded (but why does
> it still appear)?

Yeah, this confuses me, too. I built the patch for that bug on top of this one, so I guess somehow they all get combined in that same head that got pushed to reviewboard?

In any case, I'm just landing this patch on its own.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8593641 [details]
MozReview Request: bz://1155083/margaret

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

[User impact if declined]: Reader view toolbar won't be hidden properly on tablets in landscape.

[Describe test coverage new/current, TreeHerder]: Tested locally, just landed on fx-team.

[Risks and why]: Low-risk, very small CSS change.

[String/UUID change made/needed]: None.

Comment 11

[Gregory Szorc [:gps]]

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8593641 [details]
MozReview Request: bz://1155083/margaret

Let's take it. Only to m-b as reader view is disabled in m-r.

Comment 14

[Teodora Vermesan (:TeoVermesan)]

Reader view toolbar disappears when scrolling down an article on landscape, so:
Verified as fixed using:
Device: Nexus 7 (Android 5.0)
Build: Firefox for Android 40.0a1 (2015-04-23)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/20ad2caf4730

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Teodora Vermesan (:TeoVermesan)]

Reader view toolbar disappears when scrolling down an article on landscape, so:
Verified as fixed using:
Device: Nexus 7 (Android 5.0)
Build: Firefox for Android 39.0a2 (2015-04-24)

Comment 18

[Teodora Vermesan (:TeoVermesan)]

I am still able to reproduce this issue on 38 Beta 8 using Nexus 7 (Android 5.0)

Comment 19

[:Margaret Leibovic]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #18)
> I am still able to reproduce this issue on 38 Beta 8 using Nexus 7 (Android
> 5.0)

When was that beta built? Does it have this changeset in it?

Comment 20

[Ioana Chiorean]

This beta was build in our morning ( your Sunday night ) from:
mobile commit: https://hg.mozilla.org/releases/mozilla-release/rev/8fc6195511e5 

I think this was not caught as the beta was build on release channel this time ( exceptional) 

The bug it on the beta channel only - https://hg.mozilla.org/releases/mozilla-beta/pushloghtml

Comment 21

[Ryan VanderMeulen [:RyanVM]]

This bug landed on mozilla-beta for 38.0.5, not mozilla-release for 38.0. There are no builds shipping off the mozilla-beta branch at this point unless you manually download it from the FTP.

Unfortunately, status-firefox38 has two different meanings at the moment depending on the specific branch a patch has landed on. Anyway, I think this bug not being fixed on 38 Beta 8 is entirely expected. And the status flag should probably be set back to fixed to reflect that.

Comment 22

[:Margaret Leibovic]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #21)
> This bug landed on mozilla-beta for 38.0.5, not mozilla-release for 38.0.
> There are no builds shipping off the mozilla-beta branch at this point
> unless you manually download it from the FTP.
> 
> Unfortunately, status-firefox38 has two different meanings at the moment
> depending on the specific branch a patch has landed on. Anyway, I think this
> bug not being fixed on 38 Beta 8 is entirely expected. And the status flag
> should probably be set back to fixed to reflect that.

So, what do we need to do if we want this fixed in Firefox 38? I just want to avoid shipping this regression to users.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Convince Sylvestre to approve it for release. :) (I'm not release management, fwiw)

Comment 24

[:Margaret Leibovic]

Comment on attachment 8593641 [details]
MozReview Request: bz://1155083/margaret

I didn't notice that this wasn't approved for 38. Reader view never has been and never will be disabled on Android, so I would like to avoid shipping this regression in 38.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8593641 [details]
MozReview Request: bz://1155083/margaret

OK, I thought it was disabled
Should be in 38 beta 10.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/f7170ad49667

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Teodora Vermesan (:TeoVermesan)]

Reader view toolbar disappears when scrolling down an article on landscape, so:
Verified as fixed using:
Device: Sony Xperia (Android 4.4)
Build: Firefox for Android 38 Beta 10 build 2

Comment 29

[Catalin Suciu [:csuciu]]

Verified as fixed on 38.0.5b4 on Nexus 7 (5.0.2)

Comment 31

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1155083 - Properly hide reader view tablet on landscape tablets. r=bnicholson

Comment 32

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1154028 - Used scoped styles for reader view browser UI